This is an archive of the discontinued LLVM Phabricator instance.

[CompilerRT] Remove ubsan static runtime on Apple
ClosedPublic

Authored by usama54321 on Jan 11 2023, 3:17 PM.

Details

Summary

This patch removes the static ubsan runtime on Apple devices. The motivation
is to reduce the toolchain size.

Diff Detail

Event Timeline

usama54321 created this revision.Jan 11 2023, 3:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2023, 3:17 PM
Herald added a subscriber: Enna1. · View Herald Transcript
usama54321 requested review of this revision.Jan 11 2023, 3:17 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 11 2023, 3:17 PM
Herald added subscribers: Restricted Project, cfe-commits, MaskRay. · View Herald Transcript

Overall approach LGTM. I just have some very minor nits.

clang/include/clang/Basic/DiagnosticDriverKinds.td
219

Nit: Driver messages start with lowercase.

Also please check if "UBSan" is spelt differently in existing driver messages. It might actually be written more explicitly like "undefined behavior sanitizer".

clang/lib/Driver/ToolChains/Darwin.cpp
1442–1443

Maybe add an assert? As the code is constructed right now it should never fail but in the future someone could change the code and break the assumption.

  if (Sanitize.needsUbsanRt()) {
    assert(Sanitize.needsSharedRt() && "Static sanitizer runtimes not supported");
    AddLinkSanitizerLibArgs(Args, CmdArgs,
                            Sanitize.requiresMinimalRuntime() ? "ubsan_minimal"
                                                              : "ubsan");
}
compiler-rt/lib/ubsan/CMakeLists.txt
118

I think you may have accidentally added tabs here when re-indenting.

Addressing comments

usama54321 marked 3 inline comments as done.Jan 11 2023, 3:59 PM
usama54321 added inline comments.
compiler-rt/lib/ubsan/CMakeLists.txt
118

I double checked and these are spaces :/

usama54321 marked an inline comment as done.Jan 11 2023, 4:12 PM
usama54321 added inline comments.
clang/include/clang/Basic/DiagnosticDriverKinds.td
219

renamed UBSan to UndefinedBehaviorSanitizer as I see instances of AddressSanitizer and ThreadSanitizer in other messages. Did not find an equivalent for ubsan

delcypher added inline comments.Jan 11 2023, 4:14 PM
compiler-rt/lib/ubsan/CMakeLists.txt
118

Oh my bad. Maybe the >> symbol in phrabricator means indent rather than a particular space/tab choice.

This revision is now accepted and ready to land.Jan 11 2023, 5:45 PM
thetruestblue accepted this revision.Jan 17 2023, 10:19 AM

This seems reasonable to me.

This revision was landed with ongoing or failed builds.Jan 17 2023, 2:34 PM
This revision was automatically updated to reflect the committed changes.