Adding support for X86_64 UEFI target to begin with.
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
This needs a lit test.
| clang/lib/Driver/ToolChains/UEFI.cpp | ||
|---|---|---|
| 92–97 ↗ | (On Diff #528609) | This would ideally be handled by ToolChain::GetLinkerPath, can you leave a comment here along those lines? |
| 97 ↗ | (On Diff #528609) | This could be simplified as suggested, also the name of the variable should be LinkerPath. |
| 102 ↗ | (On Diff #528609) | The Environment variable is never modified so this condition will never hold. |
| clang/lib/Driver/ToolChains/UEFI.h | ||
| 10–11 ↗ | (On Diff #528609) | Nit: Can you add an empty line here? |
| 54–55 ↗ | (On Diff #528609) | Nit: Can you leave an empty line here for consistency? |
Comment Actions
Set the Objectformat in the appropriate place to fix the triple string for UEFI.
Simplify the lit test for UEFI toolchain to C.
| clang/lib/Driver/ToolChains/UEFI.cpp | ||
|---|---|---|
| 57 ↗ | (On Diff #533809) | We should be also passing -subsystem:efi_application. Note that COFF also has other types of EFI binaries: efi_boot_service_driver, efi_rom, efi_runtime_driver so we may eventually need a way to change the subsystem, but starting with -subsystem:efi_application is probably fine for now. |
| 57 ↗ | (On Diff #533809) | We also need -entry:<entry_function>. Note that the specification doesn't say what the name of the <entry_function> should be. TianoCore EDK II which is the reference UEFI implementation from Intel uses EfiMain so that might the a reasonable choice, but definitely warrants a comment and in the future we'll probably need a flag to override it. |
| 102 ↗ | (On Diff #528609) | This is still not addressed. |
| clang/lib/Driver/ToolChains/UEFI.h | ||
| 60 ↗ | (On Diff #533809) | Nit: Can you add an empty line here? |
| clang/test/Driver/uefi.c | ||
| 8 ↗ | (On Diff #533809) | This test should cover all defaults and explicit options passed to cc1 and the linker. If I run this command, I get: "/usr/local/google/home/phosek/llvm/llvm-project/build/fuchsia/bin/llvm" "clang" "-cc1" "-triple" "x86_64-unknown-uefi" "-emit-obj" "-mrelax-all" "-dumpdir" "a-" "-disable-free" "-clear-ast-before-backend" "-main-file-name" "uefi.c" "-mrelocation-model" "pic" "-pic-level" "2" "-mframe-pointer=all" "-fmath-errno" "-ffp-contract=on" "-fno-rounding-math" "-mconstructor-aliases" "-target-cpu" "x86-64" "-tune-cpu" "generic" "-debugger-tuning=gdb" "-fcoverage-compilation-dir=/usr/local/google/home/phosek/llvm/llvm-project/build/fuchsia" "-resource-dir" "/usr/local/google/home/phosek/llvm/llvm-project/build/fuchsia/lib/clang/17" "-isysroot" "/" "-fdebug-compilation-dir=/usr/local/google/home/phosek/llvm/llvm-project/build/fuchsia" "-ferror-limit" "19" "-fgnuc-version=4.2.1" "-fcolor-diagnostics" "-faddrsig" "-o" "/tmp/uefi-b7956f.o" "-x" "c" "../../clang/test/Driver/uefi.c" "/usr/local/google/home/phosek/llvm/llvm-project/build/fuchsia/./bin/lld-link" "-out:a.out" "-nologo" "/tmp/uefi-b7956f.o" We should definitely check options like -mrelocation-model, -mframe-pointer, etc. This also raises some questions, like for example should we default to -debugger-tuning=gdb for the UEFI target? |
| 9–10 ↗ | (On Diff #533809) | I don't think this is needed. |
| 11 ↗ | (On Diff #533809) | Can you ensure there's a newline at the end? |
Comment Actions
For comments that are already address, it's customary to mark those as "Done" to make it clear.
| clang/lib/Driver/ToolChains/UEFI.h | ||
|---|---|---|
| 46 ↗ | (On Diff #540091) | You can remove the IsIntegratedAssemblerDefault line. |
| 49 ↗ | (On Diff #540091) | This changed to UnwindTableLevel. https://github.com/llvm/llvm-project/commit/4388b56d525c08ce3cf941cfbad2428b0e1695b0 |
Comment Actions
Would it be possible to split up the LLVM changes into another change so we can land it separately from the Clang support?
| llvm/lib/Target/X86/X86Subtarget.h | ||
|---|---|---|
| 337 ↗ | (On Diff #557154) | It's a bit strange for this method to return true even for UEFI which is not Windows. I think it'd be clear to introduce a new method isTargetUEFI() and modify relevant callsites to use isTargetWin64() || isTargetUEFI(). Ideally that would be done as a separate patch. |
Comment Actions
Clang Driver Changes - https://reviews.llvm.org/D159541
Current - https://reviews.llvm.org/D152206
X86 Backend changes - https://reviews.llvm.org/D159540
Comment Actions
@phosek -- Regarding the abandoning commit. I had messed up the commit splitting process earlier. Uploaded the correct set of files for this change now. Please take a look.
| llvm/unittests/IR/DataLayoutTest.cpp | ||
|---|---|---|
| 108 | UEFI may be a better name in case we test more properties in the future. | |