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 reasons I am still investigating, the optimizer produced this expression in some test:

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)

LGTM!

Thanks for the explanation, it makes sense!

LGTM!

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!

In D159490#4643199, @erichkeane wrote:

Fixed in 6c0b9e357617. I didn't verify ahead of time, but hopefully the bots will yell at ME this time and I'll recognize it right away.

I think this may have broken builds:

In D159484#4641955, @probinson wrote:

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.

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

Address review comments

Address review comments

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

Hey Scott, do you think it will be possible to merge these patches soon-ish? I would like to make some further changes to the entry value side of things, and it would be nice to get this in before.

Nice catch!

LGTM!

I decided to make it a global object to guarantee its lifetime independent of any PlatformRemoteGDBServer instance. I don't mind wrapping it in a concurrent structure to guarantee thread safety though.

I am slightly wary of making this a global set without any kind of thread safe mechanism, as I (naively, not really knowing how this class is used) would expect us to be able to instantiate multiple servers and have them be accessed concurrently. As such, it makes more sense to have each server own its set of strings.

Remove duplicate line

Make a test run the verifier.

Really like the code deletions here :)

LGTM! Thanks for doing this

Everything looks good to me, this is much cleaner! On my first encounter with this class, it took me a long time to decipher what was going on, and the variant design is way more expressive.

Thank you so much for doing this! I was about to start working on this, so I'm thankful you posted this when you did! I'll have a look now

Argh, I added one more test along the same lines, gotta fix that one too

Pushed a quick fix

In D158636#4614890, @vvereschaka wrote:

Hi @fdeazeve,

the test gets failed on Aarch64 builders

• https://lab.llvm.org/buildbot/#/builders/119/builds/14812
• https://lab.llvm.org/buildbot/#/builders/119/builds/14813

please note, this builder supports only Arm/Aarch64 targets and it does not support x86_64 and any other targets.

@StephenTozer have you received any other feedback on this? I've been experimenting with this patch in the swift fork (https://github.com/apple/llvm-project) and it seems to fix all the issues I had found.

Apparently this is failing to link on the nvidia MLIR bot. Investigating

Reword a comment

In D158438#4604065, @aprantl wrote:

I wonder if these should all be constructors or static ::create() methods?