- User Since
- Mar 2 2013, 8:12 AM (221 w, 8 h)
Tue, May 23
This is good from my point of view now. You might want to wait for an ok from @spyffe.
Fri, May 19
(Having unit tests will avoid
Thanks for the review!
Depending on https://reviews.llvm.org/D33360, get rid of the verifier invocation in codegen()
Wed, May 17
Tue, May 16
This assumes that there exists a main module, which is a foreign concept to ThinLTOCodegenerator till now AFAIK. I'm not sure what you're trying to express here?
Especially, this seems used only in the codegen, which is only use in a very specific mode (i.e. split model where you store optimized bitcode and reprocess to perform only the codegen). In such case either you want to verify all the modules that you're about to codegen or none. I have the impression that you'll check only the first one. Not sure if I missed something.
What an entangled mess. This patch adds a boatload of testcases including the case where a function with broken debug info is cross-module-imported into a verified correct module. It also avoids re-verifying the module as much as possible.
Mon, May 15
Yes I think you are right. There are too many places that assume the Verifier is as function pass.
The updated patch disabled input verification in the pass manager builder and manually verifies the input.
I'm assuming the checks are removed because they are already done elsewhere?
Fri, May 12
This turns out to be quite ugly.
There are two different code paths, by their llvm-lto options:
-thinlto-action=run uses PassManagerBuilder and sets VerifyInput=true, which causes a VerifierLegacyPass to be scheduled
-thinlto-action=codegen (which mimics llc) doesn't.
The obvious correct fix for -thinlto-action=codegen is to also schedule a Verifier pass. That is easy — but wait.
Interesting. The linker invocation I was debugging was using the
I don't have an informed opinion at the moment, besides "let's benchmark it", so please go ahead for now. Once we switch over LLDB to use the LLVM DWARF parser I will likely revisit this in more depth.
I don't think that is going to work, because of the report_fatal_error that also exits.
Tue, May 9
Mon, May 8
This looks generally good (with outstanding comments addressed), thanks!
Fri, May 5
This does not really have anything to do with the code in this patch though. I just really want to understand it.
I totally understand that it doesn't need to be tracked in the MMI table and in a DEBUG_VALUE, my question was that it sounded to my like you implied that the DWARF backend actively ignores DEBUG_VALUEs pointing to static allocas which I found really surprising (as there are legitimate reasons for allocas to be described by a DEBUG_VALUE) .
Is it the right solution to use the module hash for correctness, or should the mismatch of the serialized langopts trigger a module rebuild and the module hash is only there to tune the performance/disk size tradeoff?
An llvm.dbg.declare of a static alloca is always added to the
MachineFunction dbg variable map, so these values are entirely
David, are all your concerns addressed?
Otherwise from my end this change LGTM (with all outstanding issues addressed).
Thu, May 4
By the way: you probably noticed that since last week have a brand new DWARF Verifier (accessible via dwarfdump --verify). It might be nice to develop Verifier checks alongside with the support for new sections/formats.
Seems generally fine, couple of suggestions inline.
Wed, May 3
Ah. Thanks for clarifying! We should remove it, then.
I'm not sure if globals are still reachable via !dbg, but it certainly used to be possible. Adrian, do you know the current state?
I'm not sure I understand your question:
Sure. I'm fine with a separate commit that breaks the Verifier out into its own file and also breaks the current HandleDebugLine function (which btw. should start with a lower-case character) into smaller functions.
Tue, May 2
Thanks! I'll leave this to David to mark as accepted.
Doesn't the test need some kind of REQUIRES: mips line?
Otherwise looks fine.
Mon, May 1
I think this looks much nicer!
LGTM once outstanding issues are addressed.
seems generally fine, couple of stylistic nits inline.
Fri, Apr 28
Address further review feedback from David.
Don't make anonymous namespaces distinct.
First off, thank you very much for getting this started!
Added a negative check for decl_line/decl_file to DebugInfo/Generic/namespace.ll
Add a bitcode upgrade for anonymous namespaces.
Apr 27 2017
Also, apologies for not handling Mach-O.
Typically we try to do a best effort on all supported containers (ELF, Mach-O, COFF) when adding new features.
Could you outline what is necessary there? Is it just coming up with a short-enough section name or is there more to it?
Thanks! The patch may be large, but the changes are mostly straightforward and generally look good to me. I found a couple of nits inline.
I think I may haved screwed up the phabricator process for this one.
This landed as r300523.
It would simplify fallout, reverting, triaging issues, understanding compile time impact, etc., if the debug info change was left for a separate commit (maybe waiting a day). Thoughts?
Address review feedback.
@beanz: Would sorting all these set() commands alphabetically break anything?
Apr 26 2017
Expand testcase to use more than one thrown type.
Wire up DIBuilder support & add a unittest.
@echristo: Is there anything besides LLVM tests that depends on llvm-dwarfdump's output ordering for the .gnu_pubnames section?
Apr 25 2017
Apr 21 2017
Could you also add some unit tests for the new interface?
Apr 20 2017
Besides my above comment, I'm very much looking forward to this commit. It will also help a lot with implementing dwarfdump -verify functionality.
LGTM with inline feedback addressed. Thanks!
Apr 19 2017
Rather than scanning forward for DBG_VALUEs, is there a more systematic way to find them? E.g., by iterating over USEs? If not, doing it this way is probably fine.
Address review feedback. Thanks!