Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[Basic] Support 64-bit x86 target for UEFI
ClosedPublic

Authored by Prabhuk on Jun 5 2023, 3:37 PM.

Details

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
phosek added inline comments.Jun 7 2023, 12:07 AM
clang/lib/Driver/ToolChains/UEFI.cpp
92–97

This would ideally be handled by ToolChain::GetLinkerPath, can you leave a comment here along those lines?

97

This could be simplified as suggested, also the name of the variable should be LinkerPath.

102

The Environment variable is never modified so this condition will never hold.

clang/lib/Driver/ToolChains/UEFI.h
10–11

Nit: Can you add an empty line here?

Prabhuk updated this revision to Diff 530609.Jun 12 2023, 11:25 AM

Addressed comments from last reviews except for adding lit tests.

Prabhuk updated this revision to Diff 530917.Jun 13 2023, 8:19 AM

Adding a simple lit test.

Prabhuk updated this revision to Diff 533782.Jun 22 2023, 2:18 PM

Set the Objectformat in the appropriate place to fix the triple string for UEFI.
Simplify the lit test for UEFI toolchain to C.

Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2023, 2:18 PM
Prabhuk updated this revision to Diff 533795.Jun 22 2023, 2:56 PM

Improve lit test for uefi.

Prabhuk updated this revision to Diff 533809.Jun 22 2023, 4:00 PM

Clean up lit test.

phosek added inline comments.Jun 28 2023, 11:41 PM
clang/lib/Driver/ToolChains/UEFI.cpp
58

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.

58

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

This is still not addressed.

clang/lib/Driver/ToolChains/UEFI.h
61

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?

For comments that are already address, it's customary to mark those as "Done" to make it clear.

Prabhuk updated this revision to Diff 540085.Jul 13 2023, 9:46 AM
Prabhuk marked 6 inline comments as done.

Added subsystem and entry args. Improved driver test.

Prabhuk updated this revision to Diff 540091.Jul 13 2023, 9:50 AM
Prabhuk marked 5 inline comments as done.

Minor formatting changes.

Prabhuk marked an inline comment as done.Jul 13 2023, 11:04 AM
brad added a subscriber: brad.Sep 5 2023, 11:34 PM
brad added inline comments.
clang/lib/Driver/ToolChains/UEFI.h
47

You can remove the IsIntegratedAssemblerDefault line.

50
Prabhuk updated this revision to Diff 556813.Sep 14 2023, 3:03 PM
Prabhuk marked 2 inline comments as done.

Addressed review comments.
Added "-dll" and "-tsaware:no" linker flags for UEFI.

Prabhuk updated this revision to Diff 557153.Sep 20 2023, 5:05 PM

Update X86 backend to use Microsoft ABI for UEFI target.

Prabhuk updated this revision to Diff 557154.Sep 20 2023, 5:14 PM

Minor cleanup.

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.

Prabhuk updated this revision to Diff 557202.Sep 21 2023, 1:49 PM

Splitting commit into 3 commits.

Is it possible to abandon this change?

Prabhuk updated this revision to Diff 557221.Sep 22 2023, 12:20 AM

Datalayout update.

Prabhuk updated this revision to Diff 557223.Sep 22 2023, 12:28 AM

Update patch.

@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.

phosek accepted this revision.Sep 22 2023, 12:33 AM

LGTM

This revision is now accepted and ready to land.Sep 22 2023, 12:33 AM
MaskRay accepted this revision.Sep 22 2023, 9:58 AM

DataLayout needs a unittest in llvm/unittests/IR/DataLayoutTest.cpp

Prabhuk updated this revision to Diff 557261.Sep 22 2023, 3:04 PM

Add DataLayoutTest for UEFI target.

MaskRay accepted this revision.Sep 22 2023, 3:19 PM
MaskRay added inline comments.
llvm/unittests/IR/DataLayoutTest.cpp
108 ↗(On Diff #557261)

UEFI may be a better name in case we test more properties in the future.

Prabhuk updated this revision to Diff 557263.Sep 22 2023, 3:26 PM

Updated the test name and added comment.

Prabhuk marked an inline comment as done.Sep 22 2023, 3:28 PM
This revision was landed with ongoing or failed builds.Sep 27 2023, 8:23 PM
This revision was automatically updated to reflect the committed changes.
mbrkusanin added inline comments.
llvm/unittests/IR/DataLayoutTest.cpp
109 ↗(On Diff #557430)

This is giving me a linker error:

$ ninja unittests/IR/IRTests
....
/usr/bin/ld: unittests/IR/CMakeFiles/IRTests.dir/DataLayoutTest.cpp.o: undefined reference to symbol '_ZN4llvm6TripleC1ERKNS_5TwineE'
/usr/bin/ld: ....../build/lib/libLLVMTargetParser.so.18git: error adding symbols: DSO missing from command line

Adding TargetParser to link componenets in llvm/unittests/IR/CMakeLists.txt seems to fix it for me, but can you check please?

Prabhuk added inline comments.Sep 28 2023, 7:09 AM
llvm/unittests/IR/DataLayoutTest.cpp
109 ↗(On Diff #557430)

Thanks for bringing this to my attention. I am able to build and run the IR tests from top of the tree without any failures. Let me take a closer look on how to reproduce this.

mbrkusanin added inline comments.Sep 28 2023, 7:44 AM
llvm/unittests/IR/DataLayoutTest.cpp
109 ↗(On Diff #557430)

It seems only few buildbots fail with this same error: https://lab.llvm.org/buildbot/#/builders/57/builds/30211
Not sure what is common between me and these buildbots.

But I get the same error with lld as well:

ld.lld: error: undefined symbol: llvm::Triple::Triple(llvm::Twine const&)
/>>> referenced by DataLayoutTest.cpp
/>>> unittests/IR/CMakeFiles/IRTests.dir/DataLayoutTest.cpp.o:((anonymous namespace)::DataLayoutTest_UEFI_Test::TestBody())

@Prabhuk This reproduces with my setup as well. Did you try building with -DBUILD_SHARED_LIBS=ON?

If there are others experiencing the same error then I can push my fix: https://github.com/llvm/llvm-project/pull/67696 if there are no other suggestions.

Prabhuk added inline comments.Sep 28 2023, 9:05 AM
llvm/unittests/IR/DataLayoutTest.cpp
109 ↗(On Diff #557430)

Thank you. I just reverted the change. I will reland this with an appropriate fix to the link failure.

Prabhuk reopened this revision.Sep 28 2023, 11:09 AM
This revision is now accepted and ready to land.Sep 28 2023, 11:09 AM
Prabhuk updated this revision to Diff 557456.Sep 28 2023, 11:34 AM

Relanding the change.

Relanding the change as the build dependency issue related to the
original patch is fixed by this commit:
c43aa466b8980e620fd27404ca2ba60210b76111

This revision was landed with ongoing or failed builds.Sep 28 2023, 11:36 AM
This revision was automatically updated to reflect the committed changes.