[Basic] Support 64-bit x86 target for UEFI

[Prabhuk]

Summary

Adding support for X86_64 UEFI target to begin with.

[Prabhuk]

Update commit message.

[phosek]

This needs a lit test.

clang/lib/Driver/ToolChains/UEFI.h

54–55 ↗

(On Diff #528609)

Nit: Can you leave an empty line here for consistency?

[phosek]

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?

[Prabhuk]

Addressed comments from last reviews except for adding lit tests.

[Prabhuk]

Adding a simple lit test.

[Prabhuk]

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

[Prabhuk]

Improve lit test for uefi.

[Prabhuk]

Clean up lit test.

[phosek]

clang/lib/Driver/ToolChains/UEFI.cpp

102 ↗

(On Diff #528609)

This is still not addressed.

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.

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?

[phosek]

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

[Prabhuk]

Added subsystem and entry args. Improved driver test.

[Prabhuk]

Minor formatting changes.

[brad]

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

[Prabhuk]

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

[Prabhuk]

Update X86 backend to use Microsoft ABI for UEFI target.

[Prabhuk]

Minor cleanup.

[phosek]

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]

Splitting commit into 3 commits.

[Prabhuk]

Clang Driver Changes - https://reviews.llvm.org/D159541
Current - https://reviews.llvm.org/D152206
X86 Backend changes - https://reviews.llvm.org/D159540

[phosek]

Is it possible to abandon this change?

[Prabhuk]

Datalayout update.

[Prabhuk]

Update patch.

[Prabhuk]

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

LGTM

[MaskRay]

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

[Prabhuk]

Add DataLayoutTest for UEFI target.

[MaskRay]

llvm/unittests/IR/DataLayoutTest.cpp

108

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

[Prabhuk]

Updated the test name and added comment.

[mbrkusanin]

llvm/unittests/IR/DataLayoutTest.cpp

109

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]

llvm/unittests/IR/DataLayoutTest.cpp

109

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]

llvm/unittests/IR/DataLayoutTest.cpp

109

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())

[kosarev]

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

[mbrkusanin]

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]

llvm/unittests/IR/DataLayoutTest.cpp

109

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

[Prabhuk]

Relanding the change.

[Prabhuk]

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

llvm/lib/IR/DataLayout.cpp

 //                       DataLayout Class Implementation
 //===----------------------------------------------------------------------===//
 
 const char *DataLayout::getManglingComponent(const Triple &T) {
   if (T.isOSBinFormatGOFF())
     return "-m:l";
   if (T.isOSBinFormatMachO())
     return "-m:o";
-  if (T.isOSWindows() && T.isOSBinFormatCOFF())
+  if ((T.isOSWindows() || T.isUEFI()) && T.isOSBinFormatCOFF())
     return T.getArch() == Triple::x86 ? "-m:x" : "-m:w";
   if (T.isOSBinFormatXCOFF())
     return "-m:a";
   return "-m:e";
 }
 
 static const std::pair<AlignTypeEnum, LayoutAlignElem> DefaultAlignments[] = {
     {INTEGER_ALIGN, {1, Align(1), Align(1)}},    // i1

llvm/lib/TargetParser/Triple.cpp

static Triple::ObjectFormatType getDefaultFormat(const Triple &T) {
   case Triple::aarch64:
   case Triple::aarch64_32:
   case Triple::arm:
   case Triple::thumb:
   case Triple::x86:
   case Triple::x86_64:
     if (T.isOSWindows())
       return Triple::COFF;
+    else if (T.isUEFI())
+      return Triple::COFF;
     return Triple::ELF;
 
   case Triple::aarch64_be:
   case Triple::amdgcn:
   case Triple::amdil64:
   case Triple::amdil:
   case Triple::arc:
   case Triple::armeb:

llvm/unittests/IR/DataLayoutTest.cpp

 //===- ConstantRangeTest.cpp - ConstantRange tests ------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Type.h"
+#include "llvm/TargetParser/Triple.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 namespace {
 
 TEST(DataLayoutTest, FunctionPtrAlign) {
TEST(DataLayoutTest, VectorAlign) {
   Type *const V8F32Ty = FixedVectorType::get(FloatTy, 8);
 
   // The alignment for a vector type larger than any specified vector type uses
   // the natural alignment as a fallback.
   EXPECT_EQ(Align(4 * 8), DL->getABITypeAlign(V8F32Ty));
   EXPECT_EQ(Align(4 * 8), DL->getPrefTypeAlign(V8F32Ty));
 }
 
+TEST(DataLayoutTest, UEFI) {
+  Triple TT = Triple("x86_64-unknown-uefi");
+
+  // Test UEFI X86_64 Mangling Component.
+  EXPECT_STREQ(DataLayout::getManglingComponent(TT), "-m:w");
+}
+
 } // anonymous namespace

llvm/unittests/TargetParser/TripleTest.cpp

TEST(TripleTest, ParsedIDs) {
   EXPECT_EQ(Triple::HermitCore, T.getOS());
   EXPECT_EQ(Triple::UnknownEnvironment, T.getEnvironment());
 
   T = Triple("x86_64-unknown-uefi");
   EXPECT_EQ(Triple::x86_64, T.getArch());
   EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
   EXPECT_EQ(Triple::UEFI, T.getOS());
   EXPECT_EQ(Triple::UnknownEnvironment, T.getEnvironment());
+  EXPECT_EQ(Triple::COFF, T.getObjectFormat());
 
   T = Triple("wasm32-unknown-unknown");
   EXPECT_EQ(Triple::wasm32, T.getArch());
   EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
   EXPECT_EQ(Triple::UnknownOS, T.getOS());
   EXPECT_EQ(Triple::UnknownEnvironment, T.getEnvironment());
 
   T = Triple("wasm32-unknown-wasi");