- User Since
- Mar 2 2013, 8:12 AM (228 w, 6 d)
No with testcase of sorts.
Seems reasonable, thanks!
Thu, Jul 20
Wed, Jul 19
I think this looks fine now, but I'll let Fred do the final sign off.
Thanks! Looks reasonable.
Tue, Jul 18
I think this looks good now.
Forgot to git-add new clang test case input files.
Note that this review contains also the corresponding clang changes in the tools/ directory.
Mon, Jul 17
LGTM with outstanding changes applied. Thanks!
Jun 20 2017
Seems generally straightforward/fine.
This is just a general high-level comment: When adding new DWARF5 features it might also be a good idea to think about what checks could be added to DWARFVerifier to diagnose corrupt input (and potentially implement them as well :-)
Jun 19 2017
Jun 14 2017
That said, this patch is still obviously correct.
I'm slightly confused why this works as a fix for the Debug build. Isn't this patch a no-op for a debug build?
Jun 13 2017
This is good to go now. Thanks!
Hm.. I think the other commit in the blamelist looks far more promising.
It looks like this broke the stage2 build:
Looks good. Please watch the stage2 and LTO bots carefully after committing.
Jun 12 2017
IIRC git diff -U9999 should work just fine. In any case I am not going to review the binary file — I am just going to trust that it is what you say it is in the accompanying testcase :-)
This generally LGTM, there is only a bitcode upgrade test for DW_OP_minus missing. I'm marking the review as accepted under the condition that the missing test is added before committing.
Technically this is missing a DWARF backend test, but since the next review will rewrite all DW_OP_plus to DW_OP_plus_uconst anyway, this is fine.
Jun 9 2017
Ah ok, I see. The cleanest way to do this would be to
- Implement support for DW_OP_plus_constu in LLVM
- Switch over Clang to use DW_OP_plus_constu
3a. Retire the old DW_OP_plus and implement the bitcode upgrade in LLVM
3b. Retire the old DW_OP_minus (AFAIK not used by clang) and implement the bitcode upgrade in LLVM
(The last two can be one joined commit so we only need to bump the DIExpression version number once and keep the upgrade code simple).
Thanks! I noticed that in the remainder of the patch there are still references to DW_OP_plus left, but they are completely untested. I would recommend removing all referenced to DW_OP_plus in this patch and then adding support for the "real" DW_OP_plus back in a separate commit, together with testcases.
Jun 8 2017
Almost there! Couple more comments inline.
Jun 7 2017
Jun 6 2017
Thanks, this LGTM now.
Jun 5 2017
It looks like you accidentally created a diff between your last patch and the new version. When uploading a revised patch, please always upload the diff from svn trunk.
This also needs a bitcode upgrade to transform the old use of DW_OP_minus to the new one. See my comment on D33892 for details.
Thanks, this LGTM!
Thanks, this is great! The only thing that is missing here is a bitcode upgrade that transforms the old use of DW_OP_plus to DW_OP_plus_uconst. I recommend incrementing the "Version number" in operand 0 of DIExpression to distinguish between the old and new format. You can look at r300522 for an example of how to do this.
Jun 3 2017
Thanks, I made a couple of comments inline.
Jun 1 2017
Looks good.. Are you also planning to change DIBuilder to not finalize subprograms automatically any more (and not insert them into AllSubprograms)? (That will be the more impactful change as it will force all non-clang frontends to make a similar change).
Thanks. I can commit this for you.
May 31 2017
I'm fine with this change. I'm fine with either expecting the frontends to manually call finalize on each subprogram or with having DIBuilder do it. But let's have that discussion over in https://reviews.llvm.org/D33705.
https://reviews.llvm.org/D33705 is the clang part of this. @dblaikie had some concerns (on the mailing list) - I had it only call it on cloned subprogram, but @dblaikie thought it might be more natural to call it immediately when irgen is done.
I was a bit scared by the description at first, but this is safe. Will you only invoke it on functions to be cloned in clang or on all subprograms?
May 30 2017
Thanks Keno, this appears to be a far better approach. The only question that remains is whether / how to update the linkage name of the cloned DISubprogram. But I don't think that this needs to be solved in this patch.
May 23 2017
This is good from my point of view now. You might want to wait for an ok from @spyffe.
May 19 2017
(Having unit tests will avoid
Thanks for the review!
Depending on https://reviews.llvm.org/D33360, get rid of the verifier invocation in codegen()
May 17 2017
May 16 2017
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.
May 15 2017
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?
May 12 2017
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.
May 9 2017
May 8 2017
This looks generally good (with outstanding comments addressed), thanks!
May 5 2017
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).