- User Since
- Mar 2 2013, 8:12 AM (238 w, 1 d)
Fri, Sep 22
Thu, Sep 21
Seems ok to me.
@rnk: was your concern addressed?
LGTM with inline comments addressed.
Wed, Sep 20
Rebased and addressed additional review feedback.
Testcase is the same as in the original commit?
Tue, Sep 19
Couple of (stylistic) inline comments, but otherwise looking good!
Did you accidentally upload the wrong patch?
Looks mostly good now. One more inline comment.
Mon, Sep 18
I think this works for me.
Looks like I just typed up everything that David said :-)
Added a couple more inline comments.
Doing this during parsing doesn't seem too intrusive and looks like it is the right trade-off.
Do you have a test case, too?
Fri, Sep 15
Do you think you could craft a debuginfo-tests test from PR34513, so we can avoid regressing in the future?
Thanks! Are you running the debuginfo-tests on your changes prior to committing?
Just wanted to say that I like the direction this is going and LGTM from my side once you sorted out the details David mentioned.
Thu, Sep 14
Sorry, I accidentally marked this as accepted.
Wed, Sep 13
Implement David's suggestion to skip empty non-dwo section headers in .dwo files.
use sys::path instead
Now comes with ROCK-SOLID .dwo detection!
With this update:
Tue, Sep 12
Okay, I'll implement a "don't print anything if the section doesn't exist" approach, since that seems to align best with what everybody desires. Printing an empty "contents" only if the section was explicitly required is trickier to implement, so I'll leave this for a future improvement.
With the FIXME in the code this LGTM since it leaves no doubt that we want to migrate away from it.
When dumping a .o file without a .debug_types section, should --debug-types
- print nothing
- print .debug_types:\nEOF
Address review feedback.
Okay, thanks for the explanation, Kevin!
Mon, Sep 11
Seems generally good. Since David had a lot of comments on the original review, I'll defer to him to accept this.
Fri, Sep 8
I played around with how we could generate testcases for this.
Thu, Sep 7
This seems reasonable to me, thanks!
When you commit this, could you please double-check that the tests are still running on the green dragon builders? I'll also keep an eye on them.
Currently the assumption is that you clone it under the clang/test folder, but the fact that it needs to depend on lld means that clang isn't really the best place for it. Besides, this is already broken in the mono-repo unless you manually symlink it from clang/test which seems odd.
And we should also stake out a clear path to what is necessary to make this patch obsolete and document what is missing.
Ok. I think there should be a PR to revert this change that is blocked on PR34136. The description of the option should explicitly spell out a big warning that this will introduce false positives and that it is experimental/temporary only. We should aim to get this reverted as soon as possible.
Is this obsoleted by the dbg.addr proposal or orthogonal?
Fri, Sep 1
Can this be tested?
Thu, Aug 31
Yes, looks like it is still running, it!
LGTM with outstanding issues addressed.
Looks good except for the one inline comment.
I don't understand enough about loop annotations to review this.
Okay, thanks for the explanation. In order to make this change our highest priority should be that the contract between Clang and LLVM still works.
Secondarily, we should make an effort to ensure that LLVM IR testcases that include source code actually reflect the contract and can be reproduced by recompiling the original source with clang. If the IR changes after a patch, it may make sense to clone the test so we get both coverage of the original codepath in the backend and coverage/documentation of how frontends are expected to behave.
Wed, Aug 30
Seems reasonable, thanks!
Shouldn't REQUIRES: shell be sufficient for sed?
Why does this test REQUIRE x86_64-linux?
As discussed on IRC, I think that this is *generally* a good idea, but I'm skeptical about a few issues with the concrete implementation that I commented on inline.
These changes have to be made very carefully. Have you tried running the debuginfo-tests with your patch? To do this you need to checkout the debuginfo-tests repository into the clang/test directory and have either gdb or lldb installed and working (yes it's magical and works with both!).
Tue, Aug 29
This may have gotten lost earlier: Would it be possible to instruct CloneFunction to not clone any temporary MDNodes via one of the flags that are passed to the ValueMapper?
Awesome, I can't wait for this to land!
Mon, Aug 28
Typically the answer to the question whether we should split up a patch is always yes, but I'm fine either way.
In fully-compliant mode we should just not emit any DWARF expression that ends with a DW_OP_stack_value in DWARF < 4. However, LLDB and GDB (at least the version Apple used to ship in Xcode) support a couple of expressions that are outside of the specification but do work in practice, so I'm reluctant to drop support for that by being stricter.
That said, Darwin has been using DWARF 4 for a while now, so an argument could be made that it is okay to regress on older deployment targets for the sake of correctness/standards compliance.