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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
