[PATCH] D152206: [Basic] Support 64-bit x86 target for UEFI

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/UEFI.cpp:57
+  CmdArgs.push_back("-nologo");
+
+  if (Args.hasArg(options::OPT_g_Group, options::OPT__SLASH_Z7))
----------------
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.


================
Comment at: clang/lib/Driver/ToolChains/UEFI.cpp:57
+  CmdArgs.push_back("-nologo");
+
+  if (Args.hasArg(options::OPT_g_Group, options::OPT__SLASH_Z7))
----------------
phosek wrote:
> 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.
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.


================
Comment at: clang/lib/Driver/ToolChains/UEFI.cpp:102
+      Args.MakeArgString(linkPath), CmdArgs, Inputs, Output);
+  if (!Environment.empty())
+    LinkCmd->setEnvironment(Environment);
----------------
phosek wrote:
> The `Environment` variable is never modified so this condition will never hold.
This is still not addressed.


================
Comment at: clang/lib/Driver/ToolChains/UEFI.h:60
+} // namespace clang
+#endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_UEFI_H
----------------
Nit: Can you add an empty line here?


================
Comment at: clang/test/Driver/uefi.c:8
+// CHECK: "-cc1"
+// CHECK-SAME: "-triple" "x86_64-unknown-uefi"
+// CHECK-NOT: crtend.o
----------------
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?


================
Comment at: clang/test/Driver/uefi.c:9-10
+// CHECK-SAME: "-triple" "x86_64-unknown-uefi"
+// CHECK-NOT: crtend.o
+// CHECK-NOT: crtn.o
----------------
I don't think this is needed.


================
Comment at: clang/test/Driver/uefi.c:11
+// CHECK-NOT: crtn.o
\ No newline at end of file

----------------
Can you ensure there's a newline at the end?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152206/new/

https://reviews.llvm.org/D152206