This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add support for SerenityOS
Needs ReviewPublic

Authored by ADKaster on Jul 3 2023, 6:50 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Adds support for the $arch-pc-serenity target to the Clang front end.
This makes the compiler look for libraries and headers in the right
places, and enables some security mitigations like stack-smashing
protection and position-independent code by default.

Co-authored-by: kleines Filmröllchen <filmroellchen@serenityos.org>
Co-authored-by: Andrew Kaster <akaster@serenityos.org>
Co-authored-by: Daniel Bertalan <dani@danielbertalan.dev>

Depends on D154395

Diff Detail

Event Timeline

ADKaster created this revision.Jul 3 2023, 6:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2023, 6:51 PM
ADKaster requested review of this revision.Jul 3 2023, 6:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2023, 6:51 PM

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 could have been written in a better way. There are some good ones, e.g. linux-cross.cpp.

MaskRay added inline comments.Jul 5 2023, 1:44 PM
clang/lib/Driver/ToolChains/Serenity.cpp
86

Avoid equals_insensitive. It's only recommended in some places for Windows.

phosek added a subscriber: phosek.Jul 5 2023, 2:24 PM
phosek added inline comments.
clang/lib/Driver/ToolChains/Serenity.h
39

Have you considered inheriting from Generic_GCC to allow more code reuse?

ADKaster planned changes to this revision.Jul 7 2023, 7:27 PM

Planning to rebase on top of Generic_ELF per Petr's suggestion.

clang/lib/Driver/ToolChains/Serenity.cpp
86

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

MaskRay added inline comments.Jul 8 2023, 12:07 AM
clang/lib/Driver/ToolChains/Serenity.cpp
86

I think Fuchsia should not use equals_lower. There is never a case that we'd use something like ld.LLD.

128

OPT_e has the LinkerInput flag and is rendered by AddLinkerInputs. Remove Args.AddAllArgs(CmdArgs, options::OPT_e); to avoid duplicate -e.

131

gcc doesn't recognize -Z. I think it is an early (200x) mistake that some targets render -Z. Drop it.

144

I wonder why you don't use the common addSanitizerRuntimes. ubsan runtime is normally libclang_rt.ubsan_standalone*

196

/usr/local/lib should not be in the default search path.

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

brad added a subscriber: brad.Oct 29 2023, 6:27 PM

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

ADKaster updated this revision to Diff 557927.Oct 29 2023, 7:49 PM

Base on Generic_ELF driver, add tests, add myself as co-author

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

Sounds good, done. Some of the patches didn't need updates, and others needed a rebase. I already abandoned the libcxx patch as the project still needs to figure out the proper way to provision a machine to use as a libcxx buildkite runner (and where to fund it from :) ). We'll resubmit that one on GitHub after these are in and we get that sorted out.

brad added a comment.Oct 29 2023, 9:46 PM

Make use of the concat macro with the various header and library paths.
https://github.com/llvm/llvm-project/commit/1d0cc510516d50c459f78896a0375fadb13a2b45

clang/lib/Driver/ToolChains/Serenity.cpp
78

Like the other ToolChains this should have before it..

assert((Output.isFilename() || Output.isNothing()) && "Invalid output.");

107
119
brad added a comment.Oct 29 2023, 9:49 PM

You also have to add Serenity to clang/lib/Lex/InitHeaderSearch.cpp InitHeaderSearch::ShouldAddDefaultIncludePaths().

clang/lib/Driver/ToolChains/Serenity.h
81

This just overrides the default DWARF version anyway.

ADKaster updated this revision to Diff 557945.Oct 31 2023, 10:22 AM

Brad's comments, and clang-format

ADKaster edited the summary of this revision. (Show Details)Oct 31 2023, 10:24 AM
brad added inline comments.Nov 2 2023, 3:07 AM
clang/lib/Driver/ToolChains/Serenity.cpp
203

IMO if the library path is removed then the header path should be as well.

brad added a comment.Nov 2 2023, 3:08 AM

But the rest LGTM as a start.

MaskRay added inline comments.Nov 2 2023, 10:39 AM
clang/lib/Driver/ToolChains/Serenity.cpp
32

-no-pie is nowadays canonical and GCC does't support -nopie. You can drop -nopie.

77

This is not tested

86

This is untested

212

This is for systems that historically support .ctors/.dtors https://maskray.me/blog/2021-11-07-init-ctors-init-array

If Serenity doesn't, this should be removed.

MaskRay added inline comments.Nov 2 2023, 11:13 AM
clang/lib/Driver/ToolChains/Serenity.cpp
165

crt* files are not tested

clang/test/Driver/serenity.cpp
3

https://github.com/MaskRay/Config/wiki/LLVM#clanglibdriver

The test fails in a
-DCLANG_DEFAULT_RTLIB=compiler-rt -DCLANG_DEFAULT_UNWINDLIB=libunwind -DCLANG_DEFAULT_CXX_STDLIB=libc++ build

36

https://github.com/MaskRay/Config/wiki/LLVM#clanglibdriver

The NOT -ltest fails in a
-DCLANG_DEFAULT_RTLIB=compiler-rt -DCLANG_DEFAULT_UNWINDLIB=libunwind -DCLANG_DEFAULT_CXX_STDLIB=libc++ build

brad added inline comments.Nov 2 2023, 8:19 PM
clang/lib/Driver/ToolChains/Serenity.cpp
148

Add in here..

// Silence warnings when linking C code with a C++ '-stdlib' argument.
Args.ClaimAllArgs(options::OPT_stdlib_EQ);

https://github.com/llvm/llvm-project/commit/760658c118f491985ec00f62f5a261293db67b95

ADKaster added inline comments.Nov 5 2023, 1:13 PM
clang/lib/Driver/ToolChains/Serenity.cpp
203

Fair. /usr/local is the prefix that all ports in the Ports/ tree are installed into by default (though some end up with files in /opt). We can for sure work a bit harder to make those headers/libs available to ports in our build infrastructure for them rather than putting that logic in the compiler.

212

It's my understanding that we don't currently support .ctors/.dtors, though we did a few years ago. It is a bit confusing to me how those are related to .init/.fini though. We have some stubs here for crti.S and crtn.S https://github.com/SerenityOS/serenity/blob/cf3c8a216be5aa496844aadb43ca05ad5c47bb46/Userland/Libraries/LibC/arch/x86_64/crti.S which end up giving every .so and executable a DT_INIT section, but all the actual global ctors and dtors end up in .init_array/.fini_array.

ADKaster added inline comments.Nov 5 2023, 1:54 PM
clang/lib/Driver/ToolChains/Serenity.cpp
77

Hm. this also seems like incorrect logic. In my next push I will remove this condition around --eh-frame-hdr to match the other ToolChains.

MaskRay added inline comments.Nov 5 2023, 4:29 PM
clang/lib/Driver/ToolChains/Serenity.cpp
77

https://maskray.me/blog/2020-11-08-stack-unwinding I have some notes on ".eh_frame_hdr and PT_GNU_EH_FRAME".

Clang and GCC usually pass --eh-frame-hdr to ld, with the exception that gcc -static does not pass --eh-frame-hdr. The difference is a historical choice related to __register_frame_info.

ADKaster added inline comments.Nov 5 2023, 4:46 PM
clang/lib/Driver/ToolChains/Serenity.cpp
77

Ah, this explains where @BertalanD got the original logic from, as we had a gcc port first. It was probably copied from our patched gcc spec files.

ADKaster updated this revision to Diff 558012.Nov 5 2023, 5:03 PM

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 robust, but I could be missing something

ADKaster updated this revision to Diff 558043.Nov 7 2023, 5:15 PM

test LTO with option fix

ADKaster edited the summary of this revision. (Show Details)Nov 7 2023, 5:38 PM
MaskRay added inline comments.Nov 7 2023, 10:08 PM
clang/test/Driver/serenity.cpp
21

Prefer -SAME: whenever applicable

ditto below

160

TC.GetFilePath("crti.o") returns a path to crti.o if crti.o is found, otherwise a raw crti.o without a patch component. The raw crti.o indicates that clang driver fails to find crti.o in a search path.

I think you need to change --sysroot= to pointer to a directory tree in Inputs/ where crti.o can be found.

MaskRay added inline comments.Nov 7 2023, 10:11 PM
clang/lib/Driver/ToolChains/Serenity.h
62

Test that the -cc1 line contains "-pic-level" "2" "-pic-is-pie"

69

Test that the -cc1 line contains "-funwind-tables=2"

MaskRay added inline comments.Thu, Nov 16, 1:52 PM
clang/lib/Driver/ToolChains/Serenity.cpp
26

I've simplified Gnu.cpp a bit for handling different link modes. ae623d16d50c9f12de7ae7ac1aa11c9d6857e081

52