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

MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:25 + +static bool getPIE(const ArgList , const ToolChain ) { + if (Args.hasArg(options::OPT_static, options::OPT_shared, I've simplified `Gnu.cpp` a bit for handling different

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

MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.h:61 + bool isPICDefault() const override { return true; } + bool isPIEDefault(const llvm::opt::ArgList &) const override { return true; } + bool isPICDefaultForced() const override { return

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

MaskRay added inline comments. Comment at: clang/test/Driver/serenity.cpp:20 +// SERENITY_X86_64: "{{(.*[^-.0-9A-Z_a-z])?}}ld.lld" +// SERENITY_X86_64: "-pie" +// SERENITY_X86_64: "-dynamic-linker" "/usr/lib/Loader.so" "--eh-frame-hdr" Prefer `-SAME:` whenever

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

ADKaster updated this revision to Diff 558043. ADKaster added a comment. test LTO with option fix Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154396/new/ https://reviews.llvm.org/D154396 Files: clang/lib/Basic/Targets.cpp

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

brad added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:110 +assert(!Inputs.empty() && "Must have at least one input."); +addLTOOptions(TC, Args, CmdArgs, Output, Inputs[0], + D.getLTOMode() == LTOK_Thin);

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

ADKaster updated this revision to Diff 558012. ADKaster added a comment. Add more tests and remove items per comments More tests for crt*, eh-frame-hdr, stdlib arguments remove /usr/local/include remove -fno-use-init-array claim stdlib= args remove -nopie I hope that the new tests are more

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

ADKaster added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:76 + if (!IsStatic || IsStaticPIE) +CmdArgs.push_back("--eh-frame-hdr"); + MaskRay wrote: > ADKaster wrote: > > MaskRay wrote: > > > This is not tested > > Hm. this also

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

MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:76 + if (!IsStatic || IsStaticPIE) +CmdArgs.push_back("--eh-frame-hdr"); + ADKaster wrote: > MaskRay wrote: > > This is not tested > Hm. this also seems like incorrect

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

ADKaster added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:76 + if (!IsStatic || IsStaticPIE) +CmdArgs.push_back("--eh-frame-hdr"); + MaskRay wrote: > This is not tested Hm. this also seems like incorrect logic. In my next push I

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

ADKaster added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:202 + addSystemInclude(DriverArgs, CC1Args, + concat(D.SysRoot, "/usr/local/include")); + addSystemInclude(DriverArgs, CC1Args, concat(D.SysRoot, "/usr/include"));

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

brad added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:147 + } + + if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs, Add in here.. ``` // Silence warnings when linking C code with a C++ '-stdlib' argument.

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

MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:164 +if (crtend_path.empty()) { + const char *crtend = (IsShared || IsPIE) ? "crtendS.o" : "crtend.o"; + crtend_path = TC.GetFilePath(crtend); crt* files are not

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

MaskRay added inline comments. Comment at: clang/test/Driver/serenity.cpp:2 +// UNSUPPORTED: system-windows + +/// Test a cross compiler. https://github.com/MaskRay/Config/wiki/LLVM#clanglibdriver The test fails in a `-DCLANG_DEFAULT_RTLIB=compiler-rt

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

MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:31 + Arg *Last = Args.getLastArg(options::OPT_pie, options::OPT_no_pie, + options::OPT_nopie); + return Last ? Last->getOption().matches(options::OPT_pie) : true;

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

brad added a comment. But the rest LGTM as a start. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154396/new/ https://reviews.llvm.org/D154396 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

brad added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:202 + addSystemInclude(DriverArgs, CC1Args, + concat(D.SysRoot, "/usr/local/include")); + addSystemInclude(DriverArgs, CC1Args, concat(D.SysRoot, "/usr/include"));

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

ADKaster updated this revision to Diff 557945. ADKaster added a comment. Brad's comments, and clang-format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154396/new/ https://reviews.llvm.org/D154396 Files: clang/lib/Basic/Targets.cpp

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

brad added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.h:81 + + unsigned GetDefaultDwarfVersion() const override { return 5; } + This just overrides the default DWARF version anyway. Repository: rG LLVM Github Monorepo CHANGES SINCE

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

brad added a comment. You also have to add Serenity to clang/lib/Lex/InitHeaderSearch.cpp InitHeaderSearch::ShouldAddDefaultIncludePaths(). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154396/new/ https://reviews.llvm.org/D154396

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

brad added a comment. Make use of the concat macro with the various header and library paths. https://github.com/llvm/llvm-project/commit/1d0cc510516d50c459f78896a0375fadb13a2b45 Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:78 + + if (Output.isFilename()) { +

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

ADKaster added a comment. In D154396#4655496 , @brad wrote: > In D154396#4655494 , @ADKaster > wrote: > >> @MaskRay @phosek Daniel and I have updated the patch set, Would you rather >> I update the phab patch

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

ADKaster updated this revision to Diff 557927. ADKaster added a comment. Herald added subscribers: wangpc, s.egerton, ormris, simoncook, asb. Base on Generic_ELF driver, add tests, add myself as co-author Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

brad added a comment. In D154396#4655494 , @ADKaster wrote: > @MaskRay @phosek Daniel and I have updated the patch set, Would you rather I > update the phab patch series, or re-upload the set as GitHub PRs? Update the phab patch series. Repository:

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

ADKaster added a comment. @MaskRay @phosek Daniel and I have updated the patch set, Would you rather I update the phab patch series, or re-upload the set as GitHub PRs? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154396/new/

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

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

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

ADKaster planned changes to this revision. ADKaster added a comment. Planning to rebase on top of Generic_ELF per Petr's suggestion. Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:85 + auto linkerIs = [Exec](const char *name) { +return

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

phosek added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.h:38 + +class LLVM_LIBRARY_VISIBILITY Serenity : public ToolChain { +public: Have you considered inheriting from `Generic_GCC` to allow more code reuse? Repository: rG LLVM Github

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

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

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

MaskRay added a comment. Upstreaming support requires rigid testing, otherwise it's fairly easy for other contributors to break your code while refactoring. https://maskray.me/blog/2021-08-08-toolchain-testing "I don't know whether a test is needed" `clang/test/Driver` has many tests that

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

ADKaster created this revision. Herald added a project: All. ADKaster requested review of this revision. Herald added subscribers: cfe-commits, MaskRay. Herald added a project: clang. Adds support for the `$arch-pc-serenity` target to the Clang front end. This makes the compiler look for