[PATCH] D154396: [clang] Add support for SerenityOS

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:85
+  auto linkerIs = [Exec](const char *name) {
+    return llvm::sys::path::filename(Exec).equals_insensitive(name) ||
+           llvm::sys::path::stem(Exec).equals_insensitive(name);
----------------
ADKaster wrote:
> MaskRay wrote:
> > Avoid `equals_insensitive`. It's only recommended in some places for Windows.
> What is the recommendation for this use case instead? This is the same pattern that is used by the Fuchsia driver. https://github.com/llvm/llvm-project/blob/01e3393b94d194ee99e57f23f9c08160cef94753/clang/lib/Driver/ToolChains/Fuchsia.cpp#L59-L61
> 
> though it looks like Fuchsia used that pattern back when it was called `equals_lower()`. https://github.com/llvm/llvm-project/commit/3e199ecdadf7b546054c5a5820d1678f1e83c821
I think Fuchsia should not use `equals_lower`. There is never a case that we'd use something like `ld.LLD`.


================
Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:127
+  Args.AddAllArgs(CmdArgs, options::OPT_T_Group);
+  Args.AddAllArgs(CmdArgs, options::OPT_e);
+  Args.AddAllArgs(CmdArgs, options::OPT_s);
----------------
`OPT_e` has the LinkerInput flag and is rendered by AddLinkerInputs. Remove `Args.AddAllArgs(CmdArgs, options::OPT_e);` to avoid duplicate `-e`.


================
Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:130
+  Args.AddAllArgs(CmdArgs, options::OPT_t);
+  Args.AddAllArgs(CmdArgs, options::OPT_Z_Flag);
+  Args.AddAllArgs(CmdArgs, options::OPT_r);
----------------
gcc doesn't recognize `-Z`. I think it is an early (200x) mistake that some targets render `-Z`. Drop it.


================
Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:143
+    const SanitizerArgs &Sanitize = TC.getSanitizerArgs(Args);
+    if (Sanitize.needsUbsanRt())
+      CmdArgs.push_back("-lubsan");
----------------
I wonder why you don't use the common `addSanitizerRuntimes`. ubsan runtime is normally `libclang_rt.ubsan_standalone*`


================
Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:195
+    getProgramPaths().push_back(getDriver().Dir);
+  getFilePaths().push_back(getDriver().SysRoot + "/usr/local/lib");
+  getFilePaths().push_back(getDriver().SysRoot + "/usr/lib");
----------------
`/usr/local/lib` should not be in the default search path.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154396