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).

Are you able to post the test source or an unoptimized reproducer?

In D158185#4648729, @fdeazeve wrote:

I've just noticed that, for reasons I am still investigating, the optimizer produced this expression in some test:

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:

Nice and easy

In D157615#4647235, @StephenTozer wrote:

In D157615#4646848, @dblaikie wrote:

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.

SGTM, the scope of this patch has increased as-of the latest update. That being said AFAIK Github PRs don't currently have support for stacked PRs, so I imagine I'll need to have one review up at a time.

In D159468#4646851, @dblaikie wrote:

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)

In D157615#4646848, @dblaikie wrote:

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.

In D157616#4644101, @jmorse wrote:

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.

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)

Thanks for the explanation, it makes sense!

Check for isSingleLocationExpression in more places and make other checks (isEntryValue() and similar) contingent on that check.

In D158185#4645353, @fdeazeve wrote:

Hi @StephenTozer, could you elaborate on what these failures are? I am concerned because, ever since D133926 was merged, there are some methods that were left with an incorrect implementation, like startsWithDeref or isEntryValue. It would be nice if we could address those.
Is the proposed fix here revealing a larger problem that may cause D133926 to be rethought?

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.

Rebased

In D159468#4643380, @scott.linder wrote:

In D159468#4641937, @fdeazeve wrote:

In D159468#4641242, @scott.linder wrote:

In D159468#4639858, @fdeazeve wrote:

@scott.linder This is a patch I'd like to submit in the same area where your other patches are (https://reviews.llvm.org/D158675)
I don't mind rebasing if you think they will be merged soon!

I'm just running some final tests on our CI, and hopefully all goes well and they land today. Otherwise, I am also happy to rebase after you!

Awesome! I don't mind waiting an extra day or so! :)

Thank you for waiting, and sorry it took through the weekend; I was out unexpectedly on Friday.

I just landed the series, ending with 43331461954939032a03621998c30ac90299ad40

Hopefully there are no regressions and it sticks, in which case you should be good to rebase now!

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.

In D159468#4641937, @fdeazeve wrote:

In D159468#4641242, @scott.linder wrote:

In D159468#4639858, @fdeazeve wrote:

@scott.linder This is a patch I'd like to submit in the same area where your other patches are (https://reviews.llvm.org/D158675)
I don't mind rebasing if you think they will be merged soon!

I'm just running some final tests on our CI, and hopefully all goes well and they land today. Otherwise, I am also happy to rebase after you!

Awesome! I don't mind waiting an extra day or so! :)

In D151465#4642321, @aprantl wrote:

@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?

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/59897/consoleFull#12109776838254eaf0-7326-4999-85b0-388101f2d404

@StephenTozer Broken greendragon tests temporarily XFAILed with 37a20cc68f545647e614c5ba4ae311dc3fd277e9

@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?

In D159484#4641955, @probinson wrote:

Please use GitHub for new patches.

Please use GitHub for new patches.

In D159468#4641242, @scott.linder wrote:

In D159468#4639858, @fdeazeve wrote:

@scott.linder This is a patch I'd like to submit in the same area where your other patches are (https://reviews.llvm.org/D158675)
I don't mind rebasing if you think they will be merged soon!

I'm just running some final tests on our CI, and hopefully all goes well and they land today. Otherwise, I am also happy to rebase after you!

Fix incorrect comment in commit message.

In D159468#4639858, @fdeazeve wrote:

@scott.linder This is a patch I'd like to submit in the same area where your other patches are (https://reviews.llvm.org/D158675)
I don't mind rebasing if you think they will be merged soon!

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

The reproducer in the commit message no longer asserts for me, maybe we can close this.