Two possible approaches to make this kind of problem less likely to come up again:

1. don't default the template args. might be annoying for this template.
2. replace bool with an enum class NamedForPurpose bool : { kThis = false, kThat = true }; in the template argument type

lgtm

fix typo and missing #include

This is the same as the reverted change except it always defines InstallAtExitCheckLeaks in asan_posix.cpp, just with an empty body on systems without lsan support.

Since everyone approves can someone mark it approved?

LGTM but I'll leave approval to one of the maintainers.
Thanks for the fix!

fix comment typo in earlier change

LGTM modulo clang-format. Thanks for the quick fixes!

In D87795#2289201, @vitalybuka wrote:

In D87795#2289139, @filcab wrote:

Please don’t do this. You’re adding a dependency between asan and lsan, and

This dependency already exists in asan_allocator.cpp

Also I think that duplication of these constants is not rely a problems and it's fine as-is without the patch.

In D87795#2289139, @filcab wrote:

Please don’t do this. You’re adding a dependency between asan and lsan, and
making it impossible to support platforms without lsan.

It’s a layering violation. I guess you can hack it if you add a *very
minimal * “allocator for asan and/or lsan” in sanitizer common. That
wouldn’t be ideal (a header shared between only two Sanitizers, but at
least it would be in the right place.

I redid this to put it in the __lsan namespace in lsan_common.h, which is definitely simpler all around.

update

Moved to lsan_common.h.

fix typo in windows stub

formatting

add windows stub

bad upload

add windows stub

update msg

simplified

rebase

In D87795#2278286, @vitalybuka wrote:

can you please clang format this?

rebased

bad upload

ping

Looks fine to me. Thanks!

refactored per review

They don't appear anywhere in this file, and presumably if you wanted those types you should use <stdint.h> from the compiler anyway.

I'm not 100% sure we don't need more fiddles in the driver, but we can iterate from here.

Rather than "equivalent to the resolved function at runtime", I'd say "equivalent to the resolved function as a call target". It's not equivalent to the function at runtime when the runtime use is "ptr == &func".

Even with the bugs fixed, I'm not very sanguine about truncating to 31 chars.

Refactor to share code directly with asan.

LGTM but I'm not sure who should sign off on new __has_feature symbols.

Long-term, I think we want this to be keyed entirely on a compiler predefine and not chosen in cmake.

That's a great start. IMHO we should give the driver a way to print out its embedded value too.

The proposed solution with an input linker script is a clever kludge, but it's a kludge. There's no way to make that actually general, since there is no particular named section that you can correctly assume is present in all kinds of binaries you might be linking. The kludge could be locally tailored for different cases, but that is even worse. Since the section and layout we're talking about here is entirely synthesized by the linker, it makes a lot more sense to have the linker support convenience features for using that format directly than to require users to build detailed kludges baking in the details of the linker's built-in synthesizing behavior.

obviated by -z rel

Both GNU linkers now implement -z start-stop-visibility=... and binutils 2.35 will include that feature.

First implement the lld change with standalone tests covering all cases; that's one change.
Then do the assembly bits. As others have said, follow the x86 model where @plt is required to request this reloc type.
The assembler doesn't do its own logic based on the symbols to decide what reloc types to use. The assembly syntax has to request it directly.
When the compiler is producing a reference, it explicitly uses the symbol@plt syntax (or internal equivalent for integrated-as) when a PLT entry might be required.

comment update

In D80496#2061684, @MaskRay wrote:

In D80496#2060948, @mcgrathr wrote:

LGTM. It might be worth mentioning in the doc/comment that REL format cannot pack the full range of addend values for all reloc types, but this only affects reloc types that lld doesn't support emitting as dynamic relocs since they are only used for TEXTRELs.

My understanding is that text relocations describes dynamic relocations in non-SHF_WRITE sections. I do not say text relocations because we disallow such dynamic relocations from SHF_WRITE sections as well.

LGTM. It might be worth mentioning in the doc/comment that REL format cannot pack the full range of addend values for all reloc types, but this only affects reloc types that lld doesn't support emitting as dynamic relocs since they are only used for TEXTRELs.

glibc on Linux and perhaps other platforms has long implemented pthread_cancel via unwinding, and long supported asynchronous unwinding generally for this. If LLVM intends to support the Linux glibc ABI, then it must support -fasynchronous-unwind-tables generally.

This is a direct revert of https://reviews.llvm.org/D54988. The rationale stated in that change was incorrect IMHO. When CFI is enabled, correct CFI is required, period. Making assumptions about how CFI will be used and thus when incomplete or incorrect CFI is adequate is a bad idea. The only proper distinctions are between no CFI at all, correct CFI at call return sites (-funwind-tables), and correct CFI at every instruction (-fasynchronous-unwind-tables). If generating CFI at all, then all CFI required to recover all caller registers not explicitly call-clobbered is required from at least every call return site in the function. As SCS CFI is necessary after the first or second prologue instruction, there is effectively no situation in which CFI omitting the x18 register is valid.

If people are agreed, I think separate -z rel, -z relr, and -z android-relocs flags that operate independent makes the most sense. The existing --pack-dyn-relocs can be accepted for compatibility, but need not get any new supported values.

In D79664#2028262, @MaskRay wrote:

I haven't thought much on this yet. First impression: why isn't the RELA->REL toggle a new option? It does not seem to fit with the rest of --pack-dyn-relocs which selects Android packed dynamic relocations or RELR or both.

Also note that compatible dynamic linkers support not just either, but both in the same executable (though the format requires that only one or the other be the format for PLT relocs, but those never need addends anyway). So for the large model case one could theoretically produce RELA for any relocs whose addend is too large for the bits available in the relocated entity while still producing REL for everything that fits. (The need ever to support large addends in the real world seems unlikely, but just saying.)

I'm not sure what all tests this should have. It was surprisingly easy to implement and seems to work perfectly. I've done a Fuchsia build with it that shows binaries that look as they should by eyeball, though I haven't yet tested it at runtime.

Oh, and perhaps I'll update that comment since we do know why ELF specified RELA in the first place, which is for general-case reloc types for machines not like x86, where some reloc types don't apply to locations that store as many bits as the full address size that the addend can be in the general case. When you have TEXTRELs, then any dynamic reloc can also be such a type. For newer ABIs that never allowed TEXTRELs, I think people just assumed that the encoding for dynamic relocs should match the encoding for ET_REL relocs without thinking about it deeply. In the ABIs we're concerned with today, the only narrow reloc type that might appear as a dynamic reloc is ABS32 in ELFCLASS64, but limiting addends to 32 bits is no more limiting than the rules of the small memory model, so you just wouldn't use rela->rel encoding with large memory model (or you could punt only for specific programs when you hit an out of range addend for real, which seems unlikely). (A similar constraint has always been tolerated for R_ARM_MOV[WT]* relocs where EM_ARM is canonically REL rather than RELA and so addends used in relocs for movw/movt instructions are limited to 16 bits though the address computation is always a full 32 bit computation before the reloc stores the high or low 16 bits.)

lgtm

Thanks!

lgtm

lgtm

lgtm with comment and nits

The discussion and the tests should distinguish the COMDAT semantics from the --gc-sections semantics. They are related but distinct.

I think if you provide symbols for the section bounds, they should be __start_note_gnu_build_id and __stop_note_gnu_build_id. But that doesn't seem enough of an improvement over parsing phdrs from __ehdr_start to merit the feature IMHO. Consumers still have to parse (or worse yet, assume) the note headers. What would make things trivial for consumers is a pair of symbols giving exactly the bounds of the *payload*, i.e. just the build ID itself (and presenting its variable size in a simple manner).

lgtm

A comment wouldn't hurt.

I wholeheartedly endorse documenting the policy on linker script semantics. I think the described deviation from GNU semantics for cases where explicit output section alignment doesn't jibe with input section alignment is generally fine. That is, I think this corner case is best avoided and I'm not too concerned with exactly how it goes when it happens. The warning for the case of an explicit address that is misaligned wrt input section sh_addralign is the most important thing for avoiding accidents. For the explicit alignment being less than than input requirements, the lld behavior of using the maximum of the two seems sensible to me (essentially the explicit alignment in the linker script is just another of the requirements like all the inputs' sh_addralign values and you align to the maximum of the whole set, which is perfectly intuitive). If anything, I think the GNU linkers should warn about violating input sh_addralign requirements if they're going to have that behavior.

Thanks! Can you land it for me?

Thanks! Can you land it for me?