There's definitely a lot of new findings this creates, but it's hard to say exactly how many root causes there are due to the way test failures are (not) grouped well in the way I'm testing. So far they all seem like true positives, so this would be good to submit. However a few are positive yet benign, like this interesting one (simplified):

But TLDR, either the fix in https://github.com/google/filament/pull/1566
is incorrect and the actually-bad code is elsewhere,
or you have some other unsanitized UB elsewhere. Could be both :S

In D62228#1659300, @ldionne wrote:

In D62228#1658620, @rupprecht wrote:

It looks like this breaks uses of std::vector with classes that are copy only (moves disabled): https://godbolt.org/z/ct2GIe
gcc/libstdc++ or clang/libc++ w/ exceptions enabled both support it.

Is this breakage intentional?

I don't think this was intentional, and I think your example should be valid. I added the test you provided to the test suite in https://reviews.llvm.org/rCXX371067, and I temporarily reverted this commit in https://reviews.llvm.org/rCXX371068. We can take a look once @EricWF is back from vacation.

Still think this looks good. Have you tried running this on the llvm test suite, or some other interesting corpus? Would be curious to see any pre/post patch numbers.

It looks like this breaks uses of std::vector with classes that are copy only (moves disabled): https://godbolt.org/z/ct2GIe
gcc/libstdc++ or clang/libc++ w/ exceptions enabled both support it.

Here are a couple other places using it in the wild:

• Split out separate methods for parsing brackets instead of allowing arbitrary nesting.

(Will get to the code comments later, just wanted to justify the patch)

LGTM

LGTM, once a MachO reviewer signs off. No action needed for my two comments.

Don't think I have any comments besides these two, so LGTM once they're addressed & you get a MachO reviewer to be happy w/ this.

In D66063#1636993, @jhenderson wrote:

2. Using private inheritance or making the ELFObjectFile a private member of MutableELFObject instead of a base class. This would require MutableELFObject to opt in into functions that we know to be safe to use, and wouldn't allow usage of other functions. This is safer, but could result in a fair amount of extra code. This might need to be combined with 1) too to fix the specific issues this patch addresses.

I think this patch is causing BB errors:
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/13605/steps/build%20stage1%20clang/logs/stdio

The docs don't build and rL369554 doesn't fully fix it

Whether or not this change is justified to be made into LLD, I think you'll need a llvm-readobj change to dump the note.

Namely, please add another case here: https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-readobj/ELFDumper.cpp#L4495 (and similarly for LLVMStyle::printNotes). I imagine you can repurpose the getGNUBuildId method.

FYI, you'll have to rebase this whole patch series after rL368826 due to Section::getName() changing signature

We already know that we don't want this enabled for tsan builds due to https://bugs.llvm.org/show_bug.cgi?id=42877, but I don't even know if anyone else will hit it (it's only when building one particular library).

+1 to changing report_error->reportError, same for reportWarning. I suppose it can be a followup NFC patch since it's technically consistent with the existing code. LGTM to the rest.

• Use :program: syntax everywhere
• Use `` instead of ** for llvm-ar usage

• Fix method casing
• Change class name to NameMatcher

• Use :program: rst syntax
• Correct some mistakes about tool uses
• Use :manpage: formatted link instead of :doc:

I feel a little more strongly that we need to have the toolname printed as part of the error message.

Reverted in r369167

I can't reproduce the test case -- it seems llvm-readelf already prints the NT_GNU_BUILD_ID when I follow the test steps.

We're seeing some compilation timeouts that bisect to the patch (5-10s -> >900s). I'm working on a repro.

LGTM, no concerns from me once the test is updated

Sounds like you get to claim this as a fix for https://bugs.llvm.org/show_bug.cgi?id=24115 too :)

I don't see anything major left, but please wait for James to take another look

lg, thanks!

Note that despite being a plain translation from md to rst, git doesn't think these are close enough and shows them as deletion+addition instead of moves.

Did you build these docs locally? (Honestly asking -- I'm wondering if there's something about my local setup that's overly strict, I see doc errors too frequently). I'm getting errors running ninja docs-llvm-html:

Did you run ninja docs-llvm-html (or equivalent if not using ninja) when submitting? I'm still seeing this error in trunk.