Fri, Sep 22
Code changes LGTM pending inline comments. I haven't looked at the tests yet.
(NB: I'm also going to try and get one of our buildbots building with the cmake flag set so that this doesn't immediately commit uncovered code.)
LGTM (pending clang-formatting if not already, plus a few nits).
Wed, Sep 20
Are you able to post the test source or an unoptimized reproducer?
Actually, I think that expression might be invalid? That argument 1 is supposed to mimic the DWARF specification, i.e., it attempts to specify the length of the operations that are evaluated upon function entry. We require it to be 1 because we always want to be just a "register" operation. It's not clear what the meaning would be for a variadic expression
I've just noticed that, for reason I am still investigating, the optimizer produced this expression in some test:
Mon, Sep 18
Nice and easy
Fri, Sep 15
Probably worth a FIXME and a bug filed about supporting different location kinds? (like, really - we can't describe a variable as being partly in memory, and partly in a register? That doesn't sound right to me... I'd have thought we would've had support for that a while ago, but I don't follow the variable location parts of DWARF in as much detail, so maybe it is)
Might be worth rewording the commit, or splitting it - I'd say the introduction of optdebug should be the noteworthier part of this patch (or whichever patch introduces it) - so either "this patch adds optdebug, and a first/exemplar use of it in postra scheduler" or split it into a patch that just adds the attribute and no uses, and one that adds the use. (I'd lean towards splitting it up, personally & I guess maybe moving to github pull requests in the process, perhaps)
Thu, Sep 14
Thanks for the explanation, it makes sense!
Check for isSingleLocationExpression in more places and make other checks (isEntryValue() and similar) contingent on that check.
Wed, Sep 13
However, I've found the patch triggers some errors when running on the llvm test suite, and the variadic check really isn't that expensive I think, so I'm going to rewrite the patch to do a bit more.
Tue, Sep 12
LGTM, with the rider that I'm not sure what the policy is on putting the python test in there. It's fundamentally a debug-info quality test, which is slightly different to the functional tests that we have elsewhere. The failure mode would be where some optimisation decision changes, we get a non-perfect but acceptable debug-info output, and an optimisation writer is inconvenienced by a test that "can't be correctly updated", as it were.
Mon, Sep 11
Fri, Sep 8
@StephenTozer Yes, there are green dragon errors. It looks like -lstdc++ is hardcoded. On Darwin we usually only have libc++. Could you make that configurable?
Please use GitHub for new patches.
Fix incorrect comment in commit message.
Thu, Sep 7
Since this is DWARF5/ObjectiveC only, and since it resolves more failures than it creates, I'll go ahead and merge it.
This is exposing an issue with lldb/test/API/lang/objc/modules-objc-property/TestModulesObjCProperty.py.
This test compiles an ObjectiveC program and runs dsymutil on the binary.
By default, dsymutil verifies the output only if the input is valid.
With this patch, the input is now considered valid. So dsymutil verifies the output, which turns out to be invalid (for other reasons?)
If I don't apply this patch and force dsymutil to verify the output of that test, it also says the output is invalid.
Just to register in case there are any concerns with performance here, I built Clang in Debug, generated a dsym with dsymutil build_Debug/bin/clang --accelerator Dwarf and measured the time it takes to verify it: build_Release/bin/llvm-dwarfdump --verify build_Debug/bin/clang.dSYM
Made some general fixes to tests that were broken in some capacity and addressed review comments.
Address review comments
Wed, Sep 6
The reproducer in the commit message no longer asserts for me, maybe we can close this.