Page MenuHomePhabricator

gn build: Fix support for building the builtins for baremetal.
ClosedPublic

Authored by pcc on Mar 31 2022, 8:41 PM.

Details

Summary

Our support for building for baremetal was conditional on a default
off arg and would have failed to build if you had somehow arranged
to pass the correct --target flag; presumably nobody noticed because
nobody was turning it on. A better approach is to model baremetal
as a separate "OS" called "baremetal" and build it in the same way
as we cross-compile for other targets. That's what this patch does.
I only hooked up the arm64 target but others can be added.

Diff Detail

Event Timeline

pcc created this revision.Mar 31 2022, 8:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2022, 8:41 PM
pcc requested review of this revision.Mar 31 2022, 8:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2022, 8:41 PM
thakis accepted this revision.Apr 6 2022, 6:26 AM

Seems fine. Not sure if the dep commented below should be behind a toggle. I suppose it probably builds quickly, so maybe not worth it.

llvm/utils/gn/secondary/compiler-rt/BUILD.gn
16

Will we now always build these, unconditionally?

This revision is now accepted and ready to land.Apr 6 2022, 6:26 AM
pcc added inline comments.Apr 6 2022, 10:52 AM
llvm/utils/gn/secondary/compiler-rt/BUILD.gn
16

Yes, if the aarch64 backend is enabled. We can add a toggle if we need it but it doesn't seem necessary at this point as it builds quickly and it should help avoid bitrot.

This revision was landed with ongoing or failed builds.Apr 6 2022, 11:01 AM
This revision was automatically updated to reflect the committed changes.

This broke building on non-linux: http://45.33.8.238/macm1/32578/step_4.txt

Ptal and revert if it takes a while to fix.

pcc added a comment.Apr 6 2022, 12:48 PM

This broke building on non-linux: http://45.33.8.238/macm1/32578/step_4.txt

Ptal and revert if it takes a while to fix.

D123244 fixed the problem on my M1 Mac.

This breaks building with use_asan = true afaict:

[0/1] Regenerating ninja files
ERROR at //llvm/utils/gn/build/BUILD.gn:347:5 (//llvm/utils/gn/build/toolchain:stage2_baremetal_aarch64): Assertion failed.
    assert(is_clang && (current_os == "ios" || current_os == "linux" ||
    ^-----
asan only supported on iOS/Clang, Linux/Clang, and macOS/Clang, got baremetal

(I locally added , got $current_os to the assert string)

thakis added a comment.EditedJun 15 2022, 2:30 PM

Which one is the right fix? Should we allow asan for baremetal? Or should we force off asan for the baremetal toolchain? (Diff for 2nd option would be

diff --git a/llvm/utils/gn/build/toolchain/BUILD.gn b/llvm/utils/gn/build/toolchain/BUILD.gn
index 29da00c29fca..6bf5febf3d39 100644
--- a/llvm/utils/gn/build/toolchain/BUILD.gn
+++ b/llvm/utils/gn/build/toolchain/BUILD.gn
@@ -253,6 +253,7 @@ stage2_unix_toolchain("stage2_baremetal_aarch64") {
   toolchain_args = {
     current_os = "baremetal"
     current_cpu = "arm64"
+    use_asan = false
   }
 }

)

Third option, I suppose: Don't build anything in the baremetal toolchain if use_asan is true.

pcc added a comment.Jun 15 2022, 3:52 PM

I don't think I would expect the stage 2 runtimes to be built with a sanitizer even if use_asan is set. The sanitizer runtimes themselves don't support being built with sanitizers, and they might be compiled to depend on the builtins, which would create a circular dependency. So I think use_asan and such should only affect the stage 1 compiler. I think I would favor adding use_asan = false to the stage2_unix_toolchain template.

I don't think I would expect the stage 2 runtimes to be built with a sanitizer even if use_asan is set. The sanitizer runtimes themselves don't support being built with sanitizers, and they might be compiled to depend on the builtins, which would create a circular dependency. So I think use_asan and such should only affect the stage 1 compiler. I think I would favor adding use_asan = false to the stage2_unix_toolchain template.

While it doesn't make sense to build compiler-rt with ASan, it's useful for other runtimes like libc++abi and libc++, so I'd like to avoid adding use_asan = false to the stage2_unix_toolchain template.