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

[libc] exit,abort,longjmp are noreturn #1771

Merged
merged 1 commit into from
Dec 10, 2023
Merged

Conversation

ccoffing
Copy link
Contributor

@ccoffing ccoffing commented Dec 9, 2023

If the compiler supports noreturn annotations, it can occasionally allow the optimizer to be more effective.

I checked a full build with and without this change, and didn't see any new warnings. It does save one block when building everything on the 2.88M floppy.

< blocks inuse 2229, free 624, overhead 27, total 2880
---
> blocks inuse 2228, free 625, overhead 27, total 2880

@ghaerr
Copy link
Owner

ghaerr commented Dec 9, 2023

Hello Chuck,

Well, you beat me to it on this one :) I've been meaning to make some changes like this, but didn't think it'd actually save a whole block on the distribution media, nice!

I'll commit this now, but think we could probably put the noreturn definition in features.h, sys/cdefs.h or sys/types.h, rather than a non-standard header file. This is to reduce clutter but also because there are a few more specialized ia16-elf-gcc attributes I've been thinking of trying out. Actually, the kernel would benefit from this kind of stuff as well, so it might want to go into an include/linuxmt or include/arch .h file. Libc's sys/types.h includes include/linuxmt/types.h and include/arch/types.h, perhaps one of those should include another gcc-specific include/arch/cdefs.h or similar header?

Thank you!

@ccoffing
Copy link
Contributor Author

ccoffing commented Dec 9, 2023

I debated where to put this, even what to call it. This was first standardized in the C99 spec, so this follows that lead. And modern compilers ship this header, although the syntax has changed.

N1570 Committee Draft — April 12, 2011 ISO/IEC 9899:201x
7.23 _Noreturn <stdnoreturn.h>
1 The header <stdnoreturn.h> defines the macro
noreturn
which expands to _Noreturn.

The worst thing is including what should be a compiler header in the libc includes. Maybe I don't need to supply this header at all; shouldn't the ia16 gcc include this header? It seems to... but I will have to do a super-clean build to be sure. The rest of the PR might be sufficient and standards-compliant, but we should drop the actual libc/include/stdnoreturn.h if possible.

@ccoffing
Copy link
Contributor Author

ccoffing commented Dec 9, 2023

This area of C is a mess. There are 3 options:

  • gcc has __attribute__ ((__noreturn__))
  • _Noreturn is a keyword, in C99-C23, although deprecated in C23, with the same effect.
  • noreturn is a convenience macro, but that requires including the header.

The ia16 cross compiler does include the header. Even so, I now think it would be best to sprinkle the _Noreturn keyword in the 4 libc places (and you can use it in the kernel in the future) and revert all other changes. This achieves the desired effect, but still keeping the codebase easy to build on some other compiler. Anyone building with a truly old compiler can simply -D_Noreturn=.

@ghaerr
Copy link
Owner

ghaerr commented Dec 9, 2023

Thanks for the research, I wasn't aware this (and likely many others) are C99. However, I like your implementation of noreturn being #ifdef __GNUC__ rather than _Noreturn. That said, ELKS has been heavily married to the ia16-elf-gcc compiler in a lot of ways, much like it used to be with bcc. It's still very nice to be able to run the applications (and kernel?) through another compiler e.g. clang --analyze like you've been doing.

The worst thing is including what should be a compiler header in the libc includes

Are you referring to this PR's stdnoreturn.h, or something else? Of course you've seen that ELKS libc sys/types.h also includes the kernel types.h as well as arch/types.h... so all sorts of kernel stuff gets dragged in to libc. I'm not sure its terrible that a compiler header gets included, iff it is done in only once place such that it would be easy to change.

The issue of where to put compiler-specific keyword-based enhancements should probably be thought out a bit more, but since this is the first new keyword, and first new header file, I'm OK with committing it now and possibly changing it when more keywords are added.

@ccoffing
Copy link
Contributor Author

ccoffing commented Dec 9, 2023

Are you referring to this PR's stdnoreturn.h

Yes. That much, at least, should be deleted.

@ghaerr
Copy link
Owner

ghaerr commented Dec 9, 2023

_Noreturn is a keyword, in C99-C23, although deprecated in C23, with the same effect.

Wow. Defined, then deprecated? What a mess.

I now think it would be best to sprinkle the _Noreturn keyword in the 4 libc places (and you can use it in the kernel in the future) and revert all other changes

Actually, I don't like the mess that adding underbar-prefixed keywords in actual source provides. I view this as the least best option (!)

noreturn is a convenience macro, but that requires including the header.

noreturn is so much easier to use, and allows for changing under the hood without getting into what sounds like changing C standards. And frankly, I'm not concerned about ELKS becoming "C standard" - its use of 8086 segments, __far keyword and whatnot will never be supported as a standard in "all" compilers.

gcc has attribute ((noreturn))

This also allows using #define __attribute__() to make it go away.

Anyone building with a truly old compiler can simply -D_Noreturn=.

ELKS can't be built with another compiler without lots of work.

In summary, I think we're getting close to going overboard on this. I actually prefer this first submission as-is, and would not like it to be updated to _Noreturn, with the effect of introducing more hard-to-read keywords into the source. (And I guess, in summary for the C standards, it sounds like becoming "standard" means becoming hard-to-read!!) Special underbar-prefixed keywords should definitely stay in non-kernel header files (that is, not directly in kernel headers, but in other header files) as well. To do otherwise requires changing kernel header files every time a different C compiler is used.

@ccoffing
Copy link
Contributor Author

ccoffing commented Dec 9, 2023

It builds fine after dropping the redundant stdnoreturn.h file from the PR.

@ghaerr
Copy link
Owner

ghaerr commented Dec 9, 2023

For an idea of how a great C library deals with most of this kind of stuff, see Cosmopolitan's c.inc which defines lots of convenience macros and deals with all the compiler inanities in almost a single file. c.inc is then included at a base layer that ends up in most all the header files.

noreturn used to be in this file, but I see it was recently moved to stdnoreturn.h. This was likely done for standards compliance, which you mention.

Having said all this, the aims and goals of Cosmopolitan are far greater than ELKS; I'm not sure we need to go to the same trouble.

@ghaerr
Copy link
Owner

ghaerr commented Dec 9, 2023

It builds fine after dropping the redundant stdnoreturn.h file from the PR.

Well, OK. But now haven't we made compilation (even outside of linking, just for analyzation, etc) dependent on a header file that we don't define? I actually had made a big effort to reduce this quite a bit, for complexities sake.

IIRC, ELKS is only dependent on external limits.h, stddef.h and stdint.h. (And now stdnoreturn.h).

@ccoffing
Copy link
Contributor Author

ccoffing commented Dec 9, 2023

IIRC, ELKS is only dependent on external limits.h, stddefs.h and stdint.h. (And now stdnoreturn.h).

Okay, I understand your desire now, but the list seems larger than that. :-/

$ ls cross/lib/gcc/ia16-elf/6.3.0/include | while read H ; do grep -r "include.*$H" elks libc ; done | sed -e "s/.*<//" -e "s/>.*//" | sort | uniq
float.h
stdarg.h
stdbool.h
stddef.h
stdint-gcc.h
stdint.h
stdnoreturn.h
varargs.h

@ghaerr
Copy link
Owner

ghaerr commented Dec 9, 2023

I understand your desire now, but the list seems larger than that. :-/

Thanks for the research lol!! And I thought for sure I'd killed stdbool.h (it was originally submitted as a one-off for a single use but I guess I didn't search hard/long enough).

Your list is interesting. Perhaps we should think about this a bit more. BTW, when I ended up having to add limits.h into libc for reasons I can't remember it turned into a "real pain" actually getting it to work, as my limits.h "replacement" was required for some reason and so was the original! It took forever, and finally, you can see, that libc/include/limits.h ended up having to do a #include_next <limits.h> in order to get everything to work. The moral of that story for me was to STAY AWAY from anything to do with dragging in any "standard" headers that we don't have to!!

Here's libc limits.h:

#ifndef __LIMITS_H
#define __LIMITS_H

#include <features.h>
#include __SYSINC__(limits.h)

/* Maximum number of bytes in a pathname, including the terminating null. */
#define PATH_MAX        128

/* Maximum number of bytes in a filename, not including terminating null. */
#define NAME_MAX        MAXNAMLEN

#define PIPE_BUF        PIPE_BUFSIZ

#define OPEN_MAX        NR_OPEN

#ifndef _GCC_NEXT_LIMITS_H
#include_next <limits.h>
#endif

#endif

Its a mess - we have libc limits.h, kernel limits.h, and gcc limits.h :(

@ghaerr
Copy link
Owner

ghaerr commented Dec 9, 2023

Hello @ccoffing,

The worst thing is including what should be a compiler header in the libc includes.

It got a little late last night... I now understand what you meant by the above comment - that of creating a header file with the same name as one that is supposed to be already supplied by the compiler. I agree if the header name is an actual standard, given the example of what has happened with limits.h (described above) where the compiler header was needed, along with the "replacement".

When _Noreturn was deprecated, did `<stdnoreturn.h> go away too, or is that header file the literal defining factor how to declare functions that don't return?

I'm still not sure exactly when/where #include <stdnoreturn> should be used within ELKS: there seem to be two schools of thought on this - that of having each header file include each and every other header file it needs (which I have been lately leaning towards for modularity at the expense of many more include files in each header), or having some hierarchy of header files, where some headers are seemingly automatically included (definitely more tangled, and the ELKS kernel and some libc is this way). An advantage of the former is that the header's dependencies can always be seen, while the latter usually greatly reduces the number of includes each header file needs.

Thank you!

@ghaerr ghaerr merged commit 0cf9d6a into ghaerr:master Dec 10, 2023
1 check passed
@ccoffing
Copy link
Contributor Author

@ghaerr I thought about this just a bit more. Summarizing the points...

  • You want libc to not have excessive external includes, including of the compiler.
  • I worry duplicating in libc what the compiler will provide. To duplicate it (in miniature), yet still work nicely against arbitrary user software, the libc mini-implementations of whatever feature must not only be compatible with the compiler's, but also be robust against being included before and/or after the compiler's.
  • You don't like C _SillyKeywords (nor do I) especially when they're slated to be deprecated.
  • You pointed out that gcc's attributes can be #defined away (due to the double parens). I forgot that.
  • gcc is already a requirement.

Based on that, the answer seems simple -- revert this change, and put __attribute__((__noreturn__)) at the 4 places in the libc headers.

@ghaerr
Copy link
Owner

ghaerr commented Dec 10, 2023

Summarizing the points...

I'm pretty much in agreement with your summary. But I still very much like the idea of having convenience macros - they don't have to use a special header file per se, and they can provide a simple way to hide the actual compiler extensions from the source. IMO, this is better than sprinkling compiler-specific extensions in the source, for a variety of reasons.

And what do you know - turns out I already implemented __attribute__((__no_return__)) in the kernel, and quite forgot about it! Here's include/linuxmt/kernel.h:

...

#define wontreturn          __attribute__((__noreturn__))
//#define printfesque(n)    __attribute__((__format__(__gnu_printf__, n, n + 1)))
#define printfesque(n)
...
extern void halt(void) wontreturn;
extern void panic(const char *, ...) wontreturn;
extern void printk(const char *, ...) printfesque(1);
...

I now remember doing that for some experimentation (and obviously without research like you did for proper, existing, or deprecated standards).

Based on that, the answer seems simple -- revert this change, and put attribute((noreturn)) at the 4 places in the libc headers.

I ended up committing your work yesterday because, while there is still some debate, your research showed that stdnoreturn.h is in fact a standard header file, so we're actually following a standard. But I disagree with reverting everything and adding compiler-specific keywords throughout. Instead, consider the following, which could be expanded to other keywords (notably perhaps __atttribute__(packed)):

There is already some user land stack trace code (in elkscmd/debug/) that has been partly moved to libc (which uses
-finstrument-functions-simple and uses #define noinstrument __attribute__((no_instrument_function)) in libc/misc/instrumemt.c. The noinstrument definition, as well as printfesque and a renamed wontreturn -> noreturn moved out of kernel.h could IMO be placed in a single file that could be used, for our current compiler, by both libc and kernel code.

[EDIT: After looking at Cosmopolitan libc, whose author I have great respect for, I notice that "standard" keywords have been avoided, instead using, for instance wontreturn instead of noreturn. This dodges the whole problem of dealing with changing/deprecated/unimplemeneted standard keywords or header files, while adding project-specific keywords (e.g. printfesque, wontreturn, noinstrument) that can be easily repurposed should the compiler change, as well as building in an "ANSI-only" make option, which defines each of the convenience macros to nothing. This was the path I started taking when wontreturn was added to kernel.h].

Instead of adding multiple standard or non-standard header files for some keywords or convenience macros, why not just place them in a single lower-level header file (think hierarchy includes here, instead of explicit includes everywhere), like possibly include/arch/cdefs.h. Then, in include/linuxmt/types.h do this:

#ifndef __LINUXMT_TYPES_H
#define __LINUXMT_TYPES_H

#include <arch/cdefs.h>
#include <arch/types.h>
...

Our libc sys/types.h already includes features.h which includes sys/cdefs.h, and also includes the kernel types.h using #include __SYSINC__(types.h). Thus, with a one-line change to the kernel types.h file (which is included in every kernel header one way or another), a new arch/cdefs.h file would be included in all kernel compilations and any libc compilations that include sys/types.h.

While not "standard", this would allow all ELKS to use a common convenience macro for any/all compiler-specific extensions, with very little changes to existing libc or kernel header files.

What do you think?

@ccoffing
Copy link
Contributor Author

Okay, that seems good. I should have checked what was already in the tree! Ha. I do like the thought of having it in a core header, and not having to include extra headers every time. And it can be protected to not conflict in case the compiler's version was manually included first.

Is this a change you're going to make?

@ghaerr
Copy link
Owner

ghaerr commented Dec 11, 2023

Yes @ccoffing I'll make the change, thanks for your research and comments!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants