Any chance for a review on this? The CI appears to be failing for unrelated reasons...

@mrexodia thank you for your contribution. We are currently short on reviewing time, sorry for the delay.

On what platform have you seen the need for these changes?

This being said, the PR looks useful and fine overall. However, to simplify reviewing, it would be useful to split the single commit into multiple ones with a short explanation of what each set of changes does. I would also split this into 3 PRs: one for the small LLVM 17 change (for which we also need to add a CI target), one for the smaller changes involving headers and definitions, and one for the errno changes, which might require some more discussion. Thanks again.

These changes were necessary on macos (M1) and they would also be necessary for Windows (would have to try). Generally LLVM in freestanding mode does not have access to any system headers (even stdint.h is not working properly on Linux), unless you install the cross-compilation toolchains for the platform you're targeting. I will split the pull requests and get back to you!

Why introduce errno.h in freestanding? Aren't error codes platform-dependent?

Yes, errno is platform-dependent but the functions assign to it so I had to do something 🤷‍♂️ The specific values don’t matter in this context either, it’s just to make things compile without #ifdef all over the place.

I don't see any functions touching errno in runtime/Freestanding. I'm probably missing something?

Yeah the references are in runtime/klee-libc. I just rebased on the latest master, so hopefully the CI will pass now.

Yes, putting that into klee-libc now makes more sense, thank you.

On other hand, this might pose a problem with FreeBSD - the error codes we have might not match ones used by klee-libc and we also don't have uclibc. Maybe klee-libc should still take errno.h from the host system?

I don’t think we should take anything from the host system. For example on Windows I do not have errno.h at all, which is the whole point of this PR. I checked and there are only two occurrences of errno in the klee-libc runtime, perhaps we could pass the defines manually or something?

Also the CI failure related to rlimit64 happens because the host does not have struct rlimit64 defined at all. Kind of confusing why it succeeds in master, but it’s a similar issue to the errno in my opinion (the host you compile on does not match the POSIX library implementation stubs)

For example on Windows I do not have errno.h at all

errno.h on Windows is shipped with ucrt.

perhaps we could pass the defines manually or something?

Well, the user will define them to host values in 99% of cases, I believe,

Alright, I will try to extract the 2 error codes used from the host at configure-time and put them in a generated errno.h. The issue I ran into is that the host headers are not available at all, because clang doesn’t have a sysroot for them.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.18%. Comparing base (7cc669b) to head (acafcc9).
Report is 35 commits behind head on master.

@@            Coverage Diff             @@
##           master    #1739      +/-   ##
==========================================
+ Coverage   68.98%   70.18%   +1.20%     
==========================================
  Files         162      162              
  Lines       19446    19443       -3     
  Branches     4643     4638       -5     
==========================================
+ Hits        13414    13646     +232     
+ Misses       3999     3756     -243     
- Partials     2033     2041       +8     

Alright, I rebased on the latest master and put everything in separate commits for review.

@arrowd

errno.h on Windows is shipped with ucrt.

Unless you pass the appropriate -isysroot to clang, the only headers guaranteed to be available will be the ones in <clang prefix>/lib/clang/<version>/include. On Linux I believe /usr/include is always added (at least when building for the host triplet), so things 'work' there out of the box.

@ccadar I think this is now ready for another review. Once this is merged I plan to add freestanding Windows support (which should be fairly straightforward with these changes).

The PR looks good to me overall, but I'm not a KLEE developer and can not approve.

Done!

Thanks, @mrexodia .

I see a test is failing now with ASan, can you take a look?

Thanks, @mrexodia .

I see a test is failing now with ASan, can you take a look?

The failing tests appear to be VectorInstructions/oob-read.c and kleaver/Evaluate2.kquery, which do not use any of the freestanding runtime I changed? (they do not use the runtime at all)

I think it was Feature/const_array_opt1.c, which is a time-sensitive test.
So indeed the failure seems unrelated to your changes and due to a timing issue.
I assume the last force-push did not change anything?

Yeah the last force push was just to rebase on master, nothing else was changed.

Glad to see this merged! Thanks again, @mrexodia, and also @arrowd.

🥳