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

Adjust sytem headers #8

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Adjust sytem headers #8

wants to merge 7 commits into from

Conversation

nwf
Copy link
Member

@nwf nwf commented Jun 5, 2020

While trying to build mbedtls, a number of errors were reported that ultimately stemmed from the standard headers not quite agreeing with the standard.

In one case, mbedtls #define-d ADD for its own EC math, which conflicted with the definition in cheric.h, which is basically globally in scope.

@nwf nwf requested a review from LawrenceEsswood June 5, 2020 13:32
include/stdint.h Outdated
* Useful integer type names that we can't pick up from the compile-time
* environment.
*/
typedef char int8_t;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clang (and I believe modern GCC) defines all of these for you. You get macros like __UINT8_TYPE__, and even the __UINT_LEAST16_TYPE__, __UINT_FAST32_TYPE__ etc. Given this header is wrong for 32-bit systems I would suggest just using all those; CheriOS would conceivably be highly useful for RV32 CHERI-RISC-V given its utility in embedded systems, once the issue of type bits is addressed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this just works, seconded.

Copy link
Member

@arichardson arichardson Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just do #include_next <stdint.h> to pick up the compiler-provided one? (https://github.com/CTSRD-CHERI/llvm-project/blob/master/clang/lib/Headers/stdint.h)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that works if we get rid of -nostdinc. Are we going to have -ibuiltininc soon on CTSRD-CHERI/llvm-project?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it's Darwin-only upstream.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird for that to be OS-specific. Would it be easy to add to our frontend? I feel a little jittery about removing -nostdinc from cherios. It's probably fine for my segregated build environment where the cherios builds can't see the mainline/cheribsd builds and their sysroots, but...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our usual "hack" is --sysroot=/no/such/path...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yeah, well, -nostdinc is already OS-specific, don't ask me why. There are some really bad decisions in the structuring of Clang's drivers. And no, no reason not to add it to the FreeBSD driver, though it's not strictly required and does diverge from upstream. But when Lawrence adds a CheriOS triple and driver, that driver can always support -ibuiltininc :)

@nwf nwf requested a review from jrtc27 June 5, 2020 14:02
@nwf nwf requested a review from arichardson June 6, 2020 01:33
include/stdint.h Outdated Show resolved Hide resolved
include/stdint.h Outdated
* Useful integer type names that we can't pick up from the compile-time
* environment.
*/
typedef char int8_t;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it's Darwin-only upstream.

This requires appealing to the compiler's builtin headers, which means
we have to disable -nostdinc.  Apparently there's an -ibuiltininc coming
soon, which we probably want to adopt as soon as our compiler has it.
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.

4 participants