In D40382#4188437, @marcan wrote:

FYI, this introduces a subtle regression. dlerror() calls into gettext to translate the error. ASAN itself can be initialized from a random malloc intercept which can turn out to be in gettext, which is quite common since apps initialize gettext early, and some libraries even do so in loader init calls. This ends up re-entering into gettext and corrupting a rwmutex by trying to take the write lock while the read-side is locked. The unlock sequence leaves the rwlock in a bad state. Things then deadlock much later on the bad mutex.

Switch to template wrappers

Add #if !SANITIZER_APPLE

In D146351#4207181, @yln wrote:

Hi, thanks Vitaly!

AFAIR, thread_local (and TLS in general) require macOS 10.12 / iOS 10 / watchOS 3 or greater.

Therefore, can we hold off on this a bit?

@thetruestblue is looking into bumping the minimal sanitizer deployment targets for Apple platforms. Blue, when you do that, can you circle back here and apply this patch? This change is a good example for a sanity check: it doesn't work right now but should work afterwards.

In D146324#4203326, @Chia-hungDuan wrote:

In D146324#4203093, @pcc wrote:

How do you usually build Scudo? When I run the unit tests I cross compile it for Android using llvm's gn build system. If you're building it as part of the Android platform, SCUDO_PREFIX will be defined to add a prefix of scudo_, so the alias in malloc.h wouldn't affect the definitions.

I see, thanks!

That prefix is only added when #if SCUDO_ANDROID && _BIONIC (code), do you think we need to do #if !SCUDO_ANDROID || !_BIONIC?

In D146324#4203219, @cferris wrote:

Is it worth examining how we do this on Android? For example, should we remove the mallinfo2 alias for mallinfo, so that we don't have to make this change?

How do you usually build Scudo? When I run the unit tests I cross compile it for Android using llvm's gn build system. If you're building it as part of the Android platform, SCUDO_PREFIX will be defined to add a prefix of scudo_, so the alias in malloc.h wouldn't affect the definitions.

Bring back -Wl,-z,defs for Android

After an out of band discussion with Arthur, we figured out the root cause of his problem and I was able to reproduce. This fixes it for me.

Fix commit message

To be honest, I would rather we look at removing OptimizeGlobalAliases altogether. As I explained in D65118, these kinds of trivial alias "optimizations" on their own can be more harmful than helpful.

Can't the code in CodeGenModule::HasHiddenLTOVisibility be moved here instead of duplicating it?

LGTM

Passes shouldn't be replacing aliases with aliasees in general, see D66606. The right fix is to fix SCEV to not do that.

In D65857#1621377, @peter.smith wrote:

Thanks for getting back to me, I'll look into making a strict mode. I'll also mention the use case you've identified with movk to my colleague from our GCC team, it is a pity that we don't have a R_AARCH64_PREL_G3_NC for the movk as although not ideal, as it is skipping an overflow check, it would permit GNU ld to be extended whilst retaining the property that you don't need to disassemble the binary to know how to evaluate a relocation.

I've always considered that this should be fixed by extending the resolution-based LTO API to also include resolutions for comdats.

LGTM

LGTM with nits

What do you think about making this apply not only to LTO but to other -r links as well? This would be useful for use cases like the internal symbolizer used by the sanitizers, which currently (ab)uses LTO for a similar effect. I was imagining that the user interface would be that we would make --version-script compatible with -r links, with the restriction being that it would not be possible to specify a symbol version (as this patch does).

LGTM

In D139395#4066948, @samitolvanen wrote:

Thanks for the patch, Ramon. This looks like a reasonable approach to me, and just for reference, here appears to be the corresponding rustc change:

https://github.com/rust-lang/rust/pull/105452/commits/9087c336103d0fa0b465acf8dbc1e4651250fb05

@pcc did you have any other concerns about adding this option?

In D139395#3990772, @rcvalle wrote:

I elaborated on the reasons why not use a generalized encoding in the design document in the tracking issue https://github.com/rust-lang/rust/issues/89653. The tl;dr; is that it will result in less comprehensive protection by either using a generalized encoding for all C and C++ -compiled code or across the FFI boundary, and will degrade the security of the program when linking foreign Rust-compiled code into a program written in C or C++ because the program previously used a more comprehensive encoding for all its compiled code, not fixing the issue described in the design document and the RFC https://github.com/rcvalle/rfcs/blob/improve-c-types-for-cross-language-cfi/text/0000-improve-c-types-for-cross-language-cfi.md#appendix.

A high level question is whether we want to base the cross-language encoding on Itanium at all. Itanium has concepts such as substitutions that will complicate the implementation in other languages. Encoding pointee types can also lead to complications in cross-language encodings.

In D138337#3972138, @samitolvanen wrote:

In D138337#3972009, @pcc wrote:

Can't this be implicit if LTO is being used?

I would prefer to keep this behind a flag (similarly to -mibt-seal), so we can better control when and where the optimization enabled. For example, enabling this implicitly would currently break LTO+KCFI kernel builds as the kernel has global functions that are not address-taken in C code, but still end up being indirectly called because their addresses were taken in assembly code that wasn't visible to LTO (e.g. arm64's alternative_cb mechanism).

Can't this be implicit if LTO is being used?

LGTM

Test case?

LGTM

Will @browneee be a good candidate for Data Flow Sanitizer owner as well? Note that it's fine to have a library to have more than one owner in some cases.

Does that DR apply retroactively to C++17? I get the impression that "Status: C++20" means that the issue was only fixed in C++20, which would make this well-formed with -std=c++17.

Ping.

The sanitizer bot uses a 2-stage build so I'm not sure that it can be reproduced with a single CMake invocation. You should be able to reproduce on Linux with:

git clone https://github.com/llvm/llvm-zorg.git
BUILDBOT_CLOBBER= BUILDBOT_REVISION=79ee0342dbf025bc70f237bdfe9ccb4e10a592ce llvm-zorg/zorg/buildbot/builders/sanitizers/buildbot_standard.sh

TSan builds are also broken with this change: https://lab.llvm.org/buildbot/#/builders/70/builds/28491

ld: error: undefined symbol: __tsan_func_entry
>>> referenced by cxa_aux_runtime.cpp
>>>               libcxxabi/src/CMakeFiles/cxxabi_shared_objects.dir/cxa_aux_runtime.cpp.o:(__cxa_bad_cast)
>>> referenced by cxa_aux_runtime.cpp
>>>               libcxxabi/src/CMakeFiles/cxxabi_shared_objects.dir/cxa_aux_runtime.cpp.o:(__cxa_bad_typeid)
>>> referenced by cxa_aux_runtime.cpp
>>>               libcxxabi/src/CMakeFiles/cxxabi_shared_objects.dir/cxa_aux_runtime.cpp.o:(__cxa_throw_bad_array_new_length)
>>> referenced 539 more times

(and more undefined symbols).

In phabricator those markers just mean that you added indentation, not that you added a tab.

Ping^2.

Rebased this patch by first reverting the other ubsan patches

Looks like this never got approved and a different set of patches landed instead which didn't include shared library support :(

Comment

In D128492#3606984, @MaskRay wrote:

Thanks for catching this case. For absolute symbols we can choose not relaxing in -no-pie mode. Do you prefer relaxing?

Yes, but not indirectly called from C/C++ but rather from compiler-generated code right? That's presumably why this patch + D116130 worked for @zhuhan0.

LGTM

LGTM

Okay, it seems reasonable enough to have the [[clang::lto_visibility_public]] attribute override the --lto-whole-program-visibility flag.

I don't think I would expect the stage 2 runtimes to be built with a sanitizer even if use_asan is set. The sanitizer runtimes themselves don't support being built with sanitizers, and they might be compiled to depend on the builtins, which would create a circular dependency. So I think use_asan and such should only affect the stage 1 compiler. I think I would favor adding use_asan = false to the stage2_unix_toolchain template.

In D127876#3586154, @aeubanks wrote:

In D127876#3586134, @pcc wrote:

This diverges from the documented behavior of -lto-whole-program-visibility. The flag is meant to give all classes hidden LTO visibility, but now it only does that to some of them.

perhaps I'm misunderstanding, but even with -lto-whole-program-visibility classes explicitly marked with public LTO visibility still shouldn't participate in WPD?

This diverges from the documented behavior of -lto-whole-program-visibility. The flag is meant to give all classes hidden LTO visibility, but now it only does that to some of them.

In D126089#3583129, @aeubanks wrote:

In D126089#3582321, @pcc wrote:

In D126089#3582227, @aeubanks wrote:

is this patch ok to submit at least as a short term workaround?

Chrome already has a short-term workaround, which is to disable opaque pointers, right? I think you can use that until the proper fix is developed.

As upstream tests move away from testing typed pointers, I'm worried that pinning to typed pointers will cause regressions that we don't want to spend time investigating. I'm not sure how long a redesign/rewrite would take so I'd like to get Chrome off of typed pointers soonish.