Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

i worry a bit about the proximity of "allow removing" and "make it easy for the actual Android llvm build to fuck up and accidentally ship with all the __isOSVersionAtLeast() stuff broken".

i guess i don't really understand the upside to this, and the downside for the actual shipping configuration worries me.

if this is useful, should it be a separate opt-in option?

i worry a bit about the proximity of "allow removing" and "make it easy for the actual Android llvm build to fuck up and accidentally ship with all the __isOSVersionAtLeast() stuff broken".

i guess i don't really understand the upside to this, and the downside for the actual shipping configuration worries me.

if this is useful, should it be a separate opt-in option?

+pirama for his thoughts.

i worry a bit about the proximity of "allow removing" and "make it easy for the actual Android llvm build to fuck up and accidentally ship with all the __isOSVersionAtLeast() stuff broken".

i guess i don't really understand the upside to this, and the downside for the actual shipping configuration worries me.

if this is useful, should it be a separate opt-in option?

This is important feedback and why PRs get reviewed. :)

We could put os_version_check.c behind just a non-baremetal conditional. Currently it's disabled if Fuchia is targeted, the baremetal build flag is on, or the GPU build flag is on.

The upside is that we have an initial compiler-rt without a dependency to libc, which is useful for building Bionic libc.

The includes could also be put behind the baremetal build flag instead of checking for headers. Why do the non-Android Linux includes test for <sys/auxv.h>?

The upside is that we have an initial compiler-rt without a dependency to libc, which is useful for building Bionic libc.

as the main bionic maintainer for the last 10+ years, this is the bit i don't get --- bionic builds just fine :-)

(but this is also why i added pirama, because he owns the "building llvm" side of android, and he might be familiar with whatever llvm bootstrapping problem (?) you're talking about...)

The upside is that we have an initial compiler-rt without a dependency to libc, which is useful for building Bionic libc.

as the main bionic maintainer for the last 10+ years, this is the bit i don't get --- bionic builds just fine :-)

(but this is also why i added pirama, because he owns the "building llvm" side of android, and he might be familiar with whatever llvm bootstrapping problem (?) you're talking about...)

Oops.. Bionic does use -fno-builtin. No worries about that then! I assumed it would require builtins.

Just as a side note: Doesn't libc_hwasan require you to have built compiler-rt's sanitizers that require Bionic's libc?

We've written up #127227 for the more general issue of trying to keep things acyclic.

What exactly are you worried about going wrong with a bare-metal build in a non-bare platform? Yes, I could imagine there being something that uses a privileged instruction or whatever.

Conversely, I also imagine when doing the headers trick an accidental infinite recursion, where a built-in that uses libc gets injected by the compiler into libc.

It's not obviously clear to me which of these bad things more likely.

If a new flag is the approach that is the most likely to be accepted, some way to get to the intersection of the freestanding and hosted built-ins sets (only ones that would appear in both), then I think we would be happy to contribute that.

What exactly are you worried about going wrong with a bare-metal build in a non-bare platform? Yes, I could imagine there being something that uses a privileged instruction or whatever.

For example, in compiler-rt/lib/builtins/cpu_model/aarch64.c, for baremetal targets, we skip resolving CPU features. So we might break CPU feature detection if you use the library in the wrong place. If you want a compiler-rt subset, you need to completely exclude that file.

If a new flag is the approach that is the most likely to be accepted, some way to get to the intersection of the freestanding and hosted built-ins sets (only ones that would appear in both), then I think we would be happy to contribute that.

If there's a distinct flag for it, and the resulting library has a different name from the regular compiler-rt.builtins.a, I think I'm fine with it.

Great!

That example makes sense to me --- I agree the right thing to do with the flag is indeed to drop that builtin entirely. Anything that would use it will instead gets a link error. This, we'll know when building libc against such a builtins library that we neither make dubious assumptions about CPU features, nor reenter libc in dubious ways.

and the resulting library has a different name from the regular compiler-rt.builtins.a

I am OK making the upstream build system do that if you prefer, but just so you know, we would probably keep the same name in Nixpkgs. We already use -resource-dir to carefully control when Clang sees what, and it is just easier to keep on doing that, than also patch Clang and/or LLVM to teach it about a new builtins filename.