- User Since
- Jun 28 2018, 11:39 AM (64 w, 3 d)
Mon, Sep 9
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):
Fri, Sep 6
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
Thu, Sep 5
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.
Wed, Sep 4
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.
Tue, Sep 3
Thu, Aug 29
Mon, Aug 26
Aug 23 2019
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)
Aug 22 2019
Aug 21 2019
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.
I think this patch is causing BB errors:
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.
Aug 20 2019
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
Aug 19 2019
- 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.
Aug 16 2019
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.
Aug 15 2019
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
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:
Aug 14 2019
Aug 13 2019
Did you run ninja docs-llvm-html (or equivalent if not using ninja) when submitting? I'm still seeing this error in trunk.