Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The last bits of <errno.h> work #10

Closed
wants to merge 9 commits into from

Conversation

3 participants
@le-jzr
Copy link
Contributor

commented Dec 21, 2017

TODO: more commentary
Not going to merge this at least until next week.

Basically just the conclusion of my past two weeks. Haven't tested coastline with it yet.
Note: Extra error codes from libposix are moved to <abi/errno.in> (with a fat comment explaining they are just there for convenience). It's the simplest solution, but I have a feeling others might disagree with it. Discuss in comments.

@le-jzr

This comment has been minimized.

Copy link
Contributor Author

commented Dec 24, 2017

Here goes more commentary:

First of all, I add <_bits/opaque_handle.h> which adds macro opaque_handle(type_name), which expands to a typedef of a pointer to incomplete struct type. I plan to use this in the future to define types such as VFS file handles and the like.

opaque_handle() is conditionally (#ifdef __OPAQUE_ERRNO__) used to define errno_t as a pointer type. This allows us to prevent mixing of error codes and non-error values on a type level (see my blog post).

In kernel, most syscalls return error number, but cannot be declared as returning errno_t because they are invoked indirectly from the syscall handler. For this reason sys_errno_t is additionally defined in the kernel as a type that holds error codes, but is represented as sysarg_t. There is no such type in uspace.

Pointers cannot be used by a switch, so as a workaround, there's additionally a case_errno_t (horrible name, I plan to either change it or remove it altogether, see below), defined as an alias for long, which is used for casting error codes in switch statements. Using tools/srepl '(case_errno_t) ' '', it's deliberately easy to remove from the codebase.

Additionally, a lot of binaries return errno code from main(), but at the same time also return arbitrary magic constants (-1 and small positive integers). Instead of trying to resolve this situation, I introduced a nonstandard macro EXIT_RC(rc) (currently defined in <stdlib.h> alongside EXIT_SUCCESS and EXIT_FAILURE). This macro can be changed in the future to map the codes into a reserved range, or something similar. Currently it's either a simple cast or nop.

Finally, I reassigned error code numbers to positive values, and modified libposix to use libc's errno.h.

Planned but uncommited: remove -D__OPAQUE_ERRNO__ from the default build, possibly remove the "scaffolding" as well (see below).

Before merging, there are following unresolved questions:

  • Does anyone mind having compatibility-only error constant names in core headers? It simplifies things considerably. See the comments in final abi/include/abi/errno.in.
  • Should I keep the conditional errno_t definition, case_errno_t and EXIT_RC() until we get a custom checking tool, or should I clean them out before merging into mainline?
  • If case_errno_t is kept, what form should it take? The current one is very unsemantic.
@le-jzr

This comment has been minimized.

Copy link
Contributor Author

commented Dec 24, 2017

Note: currently doesn't build on anything but amd64 and untested with coastline. I'll get to it after xmas.

@jxsvoboda

This comment has been minimized.

Copy link
Contributor

commented Dec 26, 2017

You shouldn't use double leading underscore in macro names / symbolic variables. That is the part of the namespace reserved for the compiler. Just use a single leading underscore (e.g. _errno_xx)

If you are putting all the Exxx constants in a single header split into sections, you might want to make sure that code that is not supposed to make use of a particular section cannot do so by mistake (e.g. kernel code cannot accidentaly use an error in the Posix compatibility section).

I would advice against the term 'legacy code' for ported/external software. It sort of seems to imply that software is slated to be replaced by some 'non-legacy' (HelenOS native?) software in the future, which is not necessarily the case.

@le-jzr

This comment has been minimized.

Copy link
Contributor Author

commented Dec 26, 2017

@jxsvoboda Thanks for the comments.

You shouldn't use double leading underscore in macro names / symbolic variables. That is the part of the namespace reserved for the compiler. Just use a single leading underscore (e.g. _errno_xx)

That's a bit of a misconception. Identifiers with two leading underscores, or one underscore and a capital letter, are reserved for the "language implementation", which is a fuzzy concept, but generally includes implementations of standard libraries and headers. A standards compliant program should be able to #include <errno.h> and use any non-reserved identifiers (including _errno_xx; especially _errno_xx, because single underscore prefix is generally reserved for file-level identifiers) without conflicting with anything in the header. So in short, double leading underscore is a standards-compliant solution. A single underscore isn't.

If you are putting all the Exxx constants in a single header split into sections, you might want to make sure that code that is not supposed to make use of a particular section cannot do so by mistake (e.g. kernel code cannot accidentaly use an error in the Posix compatibility section).

Good idea. It shouldn't be very difficult.

I would advice against the term 'legacy code' for ported/external software. It sort of seems to imply that software is slated to be replaced by some 'non-legacy' (HelenOS native?) software in the future, which is not necessarily the case.

You are right. I dislike this use of "legacy" myself, don't know why I wrote it like that. :/

@le-jzr

This comment has been minimized.

Copy link
Contributor Author

commented Dec 26, 2017

@jxsvoboda re: prefixes

It would make sense to use a prefix that's sure not to conflict with any future versions of any compiler, which might be something like __helenos_xxx. But in this case, I'd suggest that any existing instances of double-underscore prefix should be updated to this as well. E.g. the __errno() function in libc.

@le-jzr le-jzr force-pushed the le-jzr:todo branch 6 times, most recently from 0d9b6ac to b971252 Dec 27, 2017

@le-jzr

This comment has been minimized.

Copy link
Contributor Author

commented Dec 29, 2017

To check the massive commit e9dae33b4a0dd297560857151100e328c7f72484 in the middle:

git checkout e9dae33b4a0dd297560857151100e328c7f72484
git reset HEAD^
git add .
tools/srepl '\berrno_t\b' int
git add .
tools/srepl '\bsys_errno_t\b' sysarg_t
git add .
tools/srepl '(case_errno_t) ' ''
git reset
git diff

What remains is a just a bit of noise.

It's still possible that some ints have been changed to errno_t by accident (it was mostly done by mechanical search-and-replace), but the cases where that would go undetected are very rare and are generally limited to dead code (or code disabled via preprocessor -- using preprocessor to toggle features is Evil) and empty placeholder functions.

@le-jzr

This comment has been minimized.

Copy link
Contributor Author

commented Dec 29, 2017

I fixed issues for all configurations tested by make check, and addressed two of @jxsvoboda's suggestions. Going to test some harbor builds now, but I don't expect any problems there.

* software. These are not used in HelenOS code, but are defined here in
* order to avoid nasty hacks in libposix.
*
* If you decide to use one of these in native HelenOS code,

This comment has been minimized.

Copy link
@jermar

jermar Jan 2, 2018

Member

I think this split is not sustainable. People will simply start using those without moving them anywhere. Think of EPARTY, which was only supposed to be used by the kernel.

I also don't like that POSIX errnos are present in a HelenOS native header at all. It would be better if you could find a way to move these to libposix.

* software. These are not used in HelenOS code, but are defined here in
* order to avoid nasty hacks in libposix.
*
* If you decide to use one of these in native HelenOS code,

This comment has been minimized.

Copy link
@jermar

jermar Jan 2, 2018

Member

Same as the previous comment.

@@ -44,7 +44,7 @@
#ifdef CONFIG_SMP

/* This is only a symbol so the type is dummy. Obtain the value using &. */
extern int _hardcoded_unmapped_size;
extern errno_t _hardcoded_unmapped_size;

This comment has been minimized.

Copy link
@jermar

jermar Jan 2, 2018

Member

Why errno_t here? As the comment says, we are interested only in the address...

@@ -56,7 +56,7 @@ typedef struct {
uint32_t cpuid_edx;
} __attribute__ ((packed)) cpu_info_t;

extern int has_cpuid(void);
extern errno_t has_cpuid(void);

This comment has been minimized.

Copy link
@jermar

jermar Jan 2, 2018

Member

This is rather a true/false or no bits set / some bits set variable, there is no error information in it.

This comment has been minimized.

Copy link
@jermar

jermar Jan 2, 2018

Member

Maybe an unsigned type would be best here.

This comment has been minimized.

Copy link
@le-jzr

le-jzr Jan 2, 2018

Author Contributor

Maybe, but that's out of scope of this changeset. :)

@@ -44,7 +44,7 @@
#ifdef CONFIG_SMP

/* This is only a symbol so the type is dummy. Obtain the value using &. */
extern int _hardcoded_unmapped_size;
extern errno_t _hardcoded_unmapped_size;

This comment has been minimized.

Copy link
@jermar

jermar Jan 2, 2018

Member

Again, there is no error information in this. We only care for the address of this symbol.

@@ -47,7 +47,7 @@
*
* @return EOK on success or an error code from errno.h.
*/
int ddi_iospace_enable_arch(task_t *task, uintptr_t ioaddr, size_t size)
errno_t ddi_iospace_enable_arch(task_t *task, uintptr_t ioaddr, size_t size)
{
return 0;

This comment has been minimized.

Copy link
@jermar

jermar Jan 2, 2018

Member

return EOK?

@@ -62,7 +62,7 @@ int ddi_iospace_enable_arch(task_t *task, uintptr_t ioaddr, size_t size)
*
* @return EOK on success or an error code from errno.h.
*/
int ddi_iospace_disable_arch(task_t *task, uintptr_t ioaddr, size_t size)
errno_t ddi_iospace_disable_arch(task_t *task, uintptr_t ioaddr, size_t size)
{
return 0;

This comment has been minimized.

Copy link
@jermar

jermar Jan 2, 2018

Member

return EOK?

@@ -44,7 +44,7 @@
(ctx)->sp = ((uintptr_t) (stack)) + (size) - SP_DELTA; \
} while (0)

extern int context_save_arch(context_t *ctx) __attribute__((returns_twice));
extern errno_t context_save_arch(context_t *ctx) __attribute__((returns_twice));

This comment has been minimized.

Copy link
@jermar

jermar Jan 2, 2018

Member

This is again rather true (the context save case) / false (the context restore case) case, I don't think context_save_arch can fail.

@@ -260,7 +260,7 @@ static bool cap_reclaimer(ht_link_t *link, void *arg)
*
* @return Negative error code in case of error.

This comment has been minimized.

Copy link
@jermar

jermar Jan 2, 2018

Member

Is it still negative?

{
ddi_ioarg_t arg;
int rc = copy_from_uspace(&arg, uspace_io_arg, sizeof(ddi_ioarg_t));
errno_t rc = copy_from_uspace(&arg, uspace_io_arg, sizeof(ddi_ioarg_t));
if (rc != 0)

This comment has been minimized.

Copy link
@jermar

jermar Jan 2, 2018

Member

EOK?

{
ddi_ioarg_t arg;
int rc = copy_from_uspace(&arg, uspace_io_arg, sizeof(ddi_ioarg_t));
errno_t rc = copy_from_uspace(&arg, uspace_io_arg, sizeof(ddi_ioarg_t));
if (rc != 0)

This comment has been minimized.

Copy link
@jermar

jermar Jan 2, 2018

Member

EOK?

@@ -169,10 +169,10 @@ static kobject_ops_t phone_kobject_ops = {
*
* @return Negative error code if a new capability cannot be allocated.

This comment has been minimized.

Copy link
@jermar

jermar Jan 2, 2018

Member

Drop Negative

@@ -242,7 +242,7 @@ int main(int argc, char *argv[])
return EIO;
}

return EOK;
return 0;

This comment has been minimized.

Copy link
@jermar

jermar Jan 2, 2018

Member

This is travelling in the opposite direction.

This comment has been minimized.

Copy link
@le-jzr

le-jzr Jan 2, 2018

Author Contributor

Huh. Weird.

This comment has been minimized.

Copy link
@le-jzr

le-jzr Jan 2, 2018

Author Contributor

Ah, right, that's main(). Literal 0 is correct there, unless we make a convention of returning EXXX from main().

This comment has been minimized.

Copy link
@le-jzr

le-jzr Jan 2, 2018

Author Contributor

The story here is that I wrapped all errno returns from main in EXIT_RC() to get it to typecheck (simultaneously changing EOKs to 0 there), then I removed the macro in one of the last commits.

char key_msg[100];
int showpreview;
int classic;
errno_t showpreview;

This comment has been minimized.

Copy link
@jermar

jermar Jan 3, 2018

Member

This is actually a flag.


const struct shape *curshape;
const struct shape *nextshape;

long fallrate;
int score;
errno_t score;

This comment has been minimized.

Copy link
@jermar

jermar Jan 3, 2018

Member

An actual score, not an errno.

int Rows;
int Cols;
errno_t Rows;
errno_t Cols;

This comment has been minimized.

Copy link
@jermar

jermar Jan 3, 2018

Member

Rows and Cols are rows and cols, not errnos. Also, this is a copy of the actual file, so I guess the same issue exists in the original.

This comment has been minimized.

Copy link
@le-jzr

le-jzr Jan 3, 2018

Author Contributor

Actually, the issue is caused by it being an unbuilt copy. I forgot to correct the "demos" because they are not compiled. In fact, it seems wrong to have copies like this in the repository. What's the reason for not copying it in the build script? This way, you have to manually copy over any changes to the original.

This comment has been minimized.

Copy link
@jermar

jermar Jan 3, 2018

Member

A good point actually, I don't know why :-)


static void ahci_sata_devices_create(ahci_dev_t *, ddf_dev_t *);
static ahci_dev_t *ahci_ahci_create(ddf_dev_t *);
static void ahci_ahci_hw_start(ahci_dev_t *);

static int ahci_dev_add(ddf_dev_t *);
static errno_t ahci_dev_add(ddf_dev_t *);

This comment has been minimized.

Copy link
@jermar

jermar Jan 3, 2018

Member

I think there is a newly introduced trailing ws here

This comment has been minimized.

Copy link
@le-jzr

le-jzr Jan 3, 2018

Author Contributor

It's not new.

static int g_failure_index_selected = -1;
static errno_t g_initialized = 0;
static errno_t g_failure_index = -1;
static errno_t g_failure_index_selected = -1;

This comment has been minimized.

Copy link
@jermar

jermar Jan 3, 2018

Member

These seem like non-errno variables to me.

This comment has been minimized.

Copy link
@le-jzr

le-jzr Jan 3, 2018

Author Contributor

Hmm, yeah. Another file that is never built in HelenOS.

This comment has been minimized.

Copy link
@le-jzr

le-jzr Jan 3, 2018

Author Contributor

I need to think of an easy way to list source files that haven't been compiled during the build.

@@ -49,7 +49,7 @@ void futex_initialize(futex_t *futex, int val)

#ifdef FUTEX_UPGRADABLE

int _upgrade_futexes = 0;
errno_t _upgrade_futexes = 0;

This comment has been minimized.

Copy link
@jermar

jermar Jan 3, 2018

Member

I think this is actually a bool.

@@ -37,7 +37,7 @@ enum {
buffer_size = 16
};

static int buffer[buffer_size];
static errno_t buffer[buffer_size];

This comment has been minimized.

Copy link
@jermar

jermar Jan 3, 2018

Member

Probably not what you wanted.

@le-jzr

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2018

I'm feeling very tired today. I'll leave this until tomorrow.

@le-jzr le-jzr self-assigned this Jan 3, 2018

@le-jzr

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

I've decided to further split this into two separate merges. The first "half" will land in mainline in a few minutes.

@le-jzr le-jzr force-pushed the le-jzr:todo branch 2 times, most recently from 23b0796 to 4a03c15 Jan 4, 2018

le-jzr added some commits Jan 4, 2018

Wrap returns of `errno` from `main()` with `EXIT_RC()`.
Returns of error code from `main()` prevent type checking when `errno_t`
and `int` are considered incompatible. In order to avoid the philosophical
discussion of what should and shouldn't be returned for `main()`, we simply
wrap the error values and leave the answer to the question for future
generations to decide.
Add `#include <errno.h>` where necessary for the following commits.
It's very likely this does not account for all the places where the include
should be, but this is enough for the code to build.

@le-jzr le-jzr force-pushed the le-jzr:todo branch from 4df213f to d878abc Jan 4, 2018

le-jzr added some commits Jan 4, 2018

Use `errno_t` in all uspace and kernel code.
Change type of every variable, parameter and return value that holds an
<errno.h> constant to either `errno_t` (the usual case), or `sys_errno_t`
(some places in kernel). This is for the purpose of self-documentation,
as well as for type-checking with a bit of type definition hackery.

After this commit, HelenOS is free of code that mixes error codes with non-error
values on the assumption that error codes are negative.

@le-jzr le-jzr force-pushed the le-jzr:todo branch from d878abc to 08d4ea2 Jan 4, 2018

@le-jzr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2018

I'm closing this for now. The main problem (nonstandard error codes in libc) was resolved.
IMO we should eventually come up with a clean way to do the static checking (to make sure the self-documenting errno_t type is used where appropriate, as a matter of consistent style), but it's not a priority for the moment. In the meantime, it doesn't make much sense to keep this open.

@le-jzr le-jzr closed this Mar 8, 2018

@le-jzr le-jzr deleted the le-jzr:todo branch Mar 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.