Fix bug, add test, address review comment

This is the patch that I've been using to stress test my implementation:

diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index 6c39b8f361e7..54ea5f487ac9 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -9,6 +9,8 @@
 #ifndef SCUDO_COMBINED_H_
 #define SCUDO_COMBINED_H_

This was tested with clang but I also tried g++ 9.3.0 and it compiled there. Beyond that I reckon we should just rely on bots.

• Address review comments

• Renamed the function per feedback on the Android review

How widespread are these build systems that parse help output? (Given that it took until now to discover them, I'd venture "not very".) Maybe it would be better to fix them to explicitly pass -m and/or do something that doesn't rely on parsing help output (e.g. just try the flag and see whether it fails).

• Address review comments

In D87420#2266456, @eugenis wrote:

Where is the implementation of getPlatformTlsSlot?

Hi, thanks for getting started on upstreaming this!

Thanks and sorry for the delay.

I think that the part of the description beginning "This can be used when..." is somewhat misleading since you could pretty much say the same thing about specifying -fvisibility=hidden -fwhole-program-vtables at compile time.

FWIW, on aarch64 we decided to make -fsanitize=shadow-call-stack require the x18 reservation (instead of implying it) to try to avoid ABI mismatch problems. That is, it should be safe to mix and match code compiled with and without -fsanitize=shadow-call-stack. If we make -fsanitize=shadow-call-stack imply the x18 reservation, it makes it more likely that someone will accidentally build and link in incompatible code that does not reserve x18.

In D86166#2224751, @ro wrote:

In D86166#2224690, @pcc wrote:

@ro On D85870 you mentioned that the declaration has been changed in Solaris 11.4 to use void * which causes a build break on 11.4 due to the incompatible declaration. I checked illumos and it looks like their declaration still has caddr_t, although the comment above this declaration implies that that shouldn't matter because the system's declaration isn't visible. But what about older versions of Solaris? Presumably they still have a declaration with caddr_t so I imagine the build would break with an incompatible declaration error there. Or do we not care about older versions?

As was discussed in D84046, Solaris doesn't need an explicit declaration at all: with __EXTENSIONS__ defined, a declaration of madvise will be visible without doing anything special.

However, on Illumos with _XOPEN_SOURCE defined (as g++ does on Solaris), there is no way to make an madvise declaration visible. So on Illumos, it doesn't matter if it uses void * or caddr_t since there can be no conflict with a system declaration. However the declaration currently needs to agree with the Solaris one to avoid the build breakage.

Part of the trouble is that we currently cannot distinguish between Solaris and Illumos at all at compile time; I have a patch to define __illumos__ based on uname -o for cases where it's difficult otherwise. All a royal mess, but done for the benefit of Illumos, not for not caring about it.

With respect to other older versions of Solaris, the only one even remotely interesting (and the oldest one I still test in GCC) is 11.3. I have a bunch of patches to at least allow LLVM to compile, but there are so many issues (ld not wrapping sections in __start_<sec>/__stop_<sec> symbols, lack of __cxa_atexit, lack of fmemopen, lack of constructor priority support, and that's just the tip of the iceberg) that I strongly doubt there's any value in trying to pursue this further. In fact the 11.3 madvise declaration still using caddr_t prompted me to do the __illumos__ patch so an Illumos-only declaration wouldn't interfere with Solaris.

Add explanation to comment

@ro On D85870 you mentioned that the declaration has been changed in Solaris 11.4 to use void * which causes a build break on 11.4 due to the incompatible declaration. I checked illumos and it looks like their declaration still has caddr_t, although the comment above this declaration implies that that shouldn't matter because the system's declaration isn't visible. But what about older versions of Solaris? Presumably they still have a declaration with caddr_t so I imagine the build would break with an incompatible declaration error there. Or do we not care about older versions?

In D76802#2201164, @phosek wrote:

In D76802#2200926, @pcc wrote:

In D76802#2200906, @phosek wrote:

@davidxl does this look good to you? This feature is now gated on -counter-associated-metadata flag (I'd welcome suggestions for better name if you have some) because we require recent lld. We might be able to set that flag in the frontend when we know that the latest lld is being used in the future.

Can you find a better way of communicating this infomation than via an llvm::cl flag (e.g. use a function attribute)? The flag won't be compatible with LTO.

Do I understand your suggestion correctly that we would set function attribute when -fprofile-*-generate -fuse-lld is used that would enable the use of metadata in the backend?

In D76802#2200906, @phosek wrote:

@davidxl does this look good to you? This feature is now gated on -counter-associated-metadata flag (I'd welcome suggestions for better name if you have some) because we require recent lld. We might be able to set that flag in the frontend when we know that the latest lld is being used in the future.

LGTM

I think this is actually a different feature to D82437?

LGTM

I think you can use wc -c. See e.g. clang/test/Modules/rebuild.m.

A possible concern was that this could be constraining future development by exposing implementation details. But I think that as long as we are free to change how these options are interpreted, or start ignoring them entirely, this is probably fine.

LGTM

Where does the configuration need to live? If it just needs to be a build-time configuration, that may be simpler than exposing new mallopt options.

Any callee-saved register can be used.

Rebase

Add a tuning setting

In D72904#2164880, @MaskRay wrote:

See also @jhenderson's latest reply to the generic-abi thread "SHF_LINK_ORDER's original semantics make upgrade difficult"

# input.o:
<table header> [common]
- DW_TAG_compile_unit [common]
-- DW_TAG_variable [.data.foo]
-- DW_TAG_namespace [common]
--- DW_TAG_subprogram [.text.bar]
--- DW_TAG_variable [.data.baz]
<table footer> [common]

@pcc I sent the patches since you expressed a preference on more work on SHF_LINK_ORDER. Though, @jhenderson's needs (and my header/footer argument in that thread) make me think more whether we should continue patching SHF_LINK_ORDER or start pursuing another section flag.

Without metadata sections's override, SHF_LINK_ORDER will have very few use cases. The problems still exist but we probably don't really need to address them.

Doesn't this require a change to the parser to accept 0 in place of the linked symbol?

In D82251#2106010, @jhenderson wrote:

Seems reasonable, but the llvm-nm documentation needs updating (see llvm/docs/CommandGuide/llvm-nm.rst).

It may also be worth release-noting the change in behaviour too, since it will mean current ARM users who use llvm-nm will see their mapping symbols disappear from the output, if they aren't already using --special-syms.

Win32 has an API for disabling error reporting which is already implemented in sys::DisableSystemDialogsOnCrash(), gtest and relevant test cases such as compiler-rt/test/asan/TestCases/ill.cpp.

LGTM

Should we drop this and just let users set sh_link=self if they want these semantics?

In D80186#2068055, @phosek wrote:

In D80186#2067298, @pcc wrote:

Yeah. Thinking about it more, you may consider a different !associated scheme for the __prof*_ globals.

• __profc_* and __profn_* are associated with a dummy global (to allow the sections to be GC'd).
• __profd_* is associated with the corresponding __profc_* section.

Now no special rules are required for inlining. __profc_* is kept alive via the references in the functions, __profd_ is kept alive via !associated and __profn_* is kept alive by direct reference.

I tried that, but now we get an error ld.lld: error: /tmp/instrprof-gc-sections-5f007f.o:(__llvm_prf_cnts): sh_link points to discarded section /tmp/instrprof-gc-sections-5f007f.o:(__llvm_prf_dummy) because the dummy global was discarded which creates a broken sh_link.

Yeah. Thinking about it more, you may consider a different !associated scheme for the __prof*_ globals.

In this case you might consider replacing the global reference with null and implementing my proposal in https://bugs.llvm.org/show_bug.cgi?id=41734#c2 to make the section SHF_LINK_ORDER with a zero-length .init_array section.

And instead of returning (global_begin, global_end) you could return something like an ArrayRef<hwasan_global> (I don't think we have ArrayRef in compiler-rt, but you could add one, it's pretty simple). Now the client code just looks like

for (hwasan_global &g : HwasanGlobalsFor(phdr, ...)) {
 ...
}

In D80599#2058889, @hctim wrote:

In D80599#2056315, @pcc wrote:

Instead of adding these callbacks would it be simpler to have a function that takes the pointer to phdrs and returns (global_begin, global_end) (both 0 if not present)?

That would require duplicating the global iteration logic across the reporting and initialisation paths, which I was trying to avoid. Having a callback that recieves a single global is easier for my brain, but please let me know if you'd prefer it the other way around.

LGTM

Instead of adding these callbacks would it be simpler to have a function that takes the pointer to phdrs and returns (global_begin, global_end) (both 0 if not present)?

LGTM

In D79818#2035301, @phosek wrote:

In D79818#2035232, @MaskRay wrote:

@mehdi_amini @pcc @tejohnson

I just saw another issue saying that clang -Os -flto=thin a.c does not work https://bugs.llvm.org/show_bug.cgi?id=45913 (+ @manojgupta)
I marked it as yet another duplicate of PR42445

For -Os, clang driver passes -plugin-opt=Os (https://github.com/llvm/llvm-project/blob/master/clang/lib/Driver/ToolChains/CommonArgs.cpp#L402) but neither LLVMgold.so nor LLD recognizes -plugin-opt=Os.
-Oz is similar. What is the concrete suggestion here? For quick reference, the -plugin-opt=O logic was added by rL256146.

The issue form my perspective is the inconsistency. Today, lld accepts -plugin-opt=O<number> but it doesn't accept -plugin-opt=O<letter>, but Clang automatically translates -O<anything> to -plugin-opt=O<anything> which results in a failure when your try to use -Os, -Oz or -Og with (Thin)LTO.

Not sure if there's a good way to test this @eugenis?

It is unclear to me whether removing the nounwind condition is necessary. The wording in the langref talks about "asynchronous exceptions" but only provides semantics for "exception handling schemes that are recognized by LLVM to handle asynchronous exceptions, such as SEH". Since we don't support anything like that on non-Windows. I don't believe that we need to support throwing exceptions past such functions on DWARF platforms, and the unwind info can only be used for the purpose of creating a stack trace.

LGTM

The architecture may need to be different here IMHO because of subsections. Don't forget that you need to map relocations onto subsections in order to implement gc-sections, and depending on the number of subsections you have per section, that could get expensive without an intermediate data structure. On top of that you'd still need the O(M log N) at output time. To me it seemed better to pay the O(M log N) up front once and avoid the cost at gc-sections time.