Page MenuHomePhabricator

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

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

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

Details

Reviewers
phosek
MaskRay
Summary

Adding support for X86_64 UEFI target to begin with.

Diff Detail

Event Timeline

Prabhuk created this revision.Jun 5 2023, 3:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2023, 3:37 PM
Herald added a subscriber: pengfei. · View Herald Transcript
Prabhuk requested review of this revision.Jun 5 2023, 3:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2023, 3:37 PM
Prabhuk updated this revision to Diff 528609.Jun 5 2023, 3:39 PM

Update commit message.

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?

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
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?

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.Tue, Sep 5, 11:34 PM
brad added inline comments.
clang/lib/Driver/ToolChains/UEFI.h
46 ↗(On Diff #540091)

You can remove the IsIntegratedAssemblerDefault line.

49 ↗(On Diff #540091)
Prabhuk updated this revision to Diff 556813.Thu, Sep 14, 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.Wed, Sep 20, 5:05 PM

Update X86 backend to use Microsoft ABI for UEFI target.

Prabhuk updated this revision to Diff 557154.Wed, Sep 20, 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.Thu, Sep 21, 1:49 PM

Splitting commit into 3 commits.

Is it possible to abandon this change?

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

Datalayout update.

Prabhuk updated this revision to Diff 557223.Fri, Sep 22, 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.Fri, Sep 22, 12:33 AM

LGTM

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

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

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

Add DataLayoutTest for UEFI target.

MaskRay accepted this revision.Fri, Sep 22, 3:19 PM
MaskRay added inline comments.
llvm/unittests/IR/DataLayoutTest.cpp
108

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

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

Updated the test name and added comment.

Prabhuk marked an inline comment as done.Fri, Sep 22, 3:28 PM