I like that this makes getLineNumber behave less "magic". I'm slightly worried that this isn't quite as NFC as think (and the other cases are just not caught by the testsuite). Perhaps you could compile Clang (and a sample of Clang's ObjC(xx) tests) with the patch and diff the dwarfdump output?

It sounds like the bug @dongAxis1944 has found is unrelated to this patch. Should we move forward with this one, or are there any other problems you see?

Do we understand *why* the frontend is emitting the variables twice? If that just a bug in the clang frontend (which would be my first guess) then we should aim at fixing the frontend instead. If the frontend is trying to do something different, I would love to understand what that is.

Thanks, I can reproduce the the issue! However, it looks like it reproduces even without my patch. The frontend (or perhaps CoroSplit?) apparently insert both a "t" function argument and a "t" local variable in the same scope:

I think this looks reasonable!

Thanks for the explanation!

That's even better!

Based on your explanation, this seems good. Thanks!

What's the testing story for WASM going to be?

Do you have any plan to salvage debug info under O2 when most of the dbg.declare are changed to dbg.value?

I think the patch might occur an error when compilling the following code (commit id of clang is 9d70dbdc2bf294abffd4b2c9ae524055f72d017a).

Mechanically this LGTM.

this lgtm with all other reviewer's concerns addressed.

the fragmenting code would need to take the pointer size into account (one of the bugs I want to solve is that we're always assuming a 64-bit stack, whereas dwarf says it's address-sized). Knowing the address size at some point is unavoidable, so this might not be a big issue.

Thanks, Pavel! My aspiration for DwarfExpression.cpp was to eventually add some backtracking capability to it to choose the optimal (=shortest) sequence of instructions for each expression. So far we haven't done this, because local decisions were usually sufficient, and more importantly, streaming the output limited our flexibility. If we give DwarfExpression a one-step look-ahead buffer, we could implement addConstantFP() as pushing two alternatives to the buffer:

Thank you, this makes sense!

Fix a potential cast error.

This is needed because representation and specification (types of operands) of

it a bit different than actual dwarf expression DW_OP_implicit_pointer.

In D92013#2413734, @labath wrote:

I'd like to also fix this in a more fundamental way, but I think I'd need some guidance from someone more familiar with this code (who would that be?).

One idea I had is to add something like isPureFragment() to the DIExpression class, which would return true if the expression consistent solely of a single DW_OP_LLVM_fragment operand.

Can replaceDbgUsesWithUndef from Local.cpp be used here?

I think you accidentally deleted your patch :-)

Mechanically LGTM to me (one question inline).

Although — no that I read the reply — it looks like the MDTuple question hasn't actually been answered ;-)

I'm sorry, I just accidentally replied to an old mail without realizing that this question was already answered weeks ago.

I think this would work. Could you base it on MDTuple and would that simplify the implementation?

Mechanically this looks fine.

LGTM once all outstanding comments are addressed.

Nice!

I think this looks reasonable.

lgtm

This is fine apart from some nitpicks inside.

This is nice!

Thanks!

Should we add __swift_ast, too?

Nice!

Just FYI, we do have unit tests for the crashlog parser now, but I don't know how well this fits in there.

Landed in 0b2b50a5d2893af47466f191771d20c9993a1624

0b2b50a5d2893af47466f191771d20c9993a1624

Duncan, did you intentionally add yourself as a blocking reviewer or is this perhaps a misconfigured Herald rule?

Also, I'm sorry for being so slow at reviewing these days...

Not an expert either, but at a high level this seems reasonable.

The push_back(nullptr) reads weird to me, but it is an accurate reflection of what the IR will look like.

This looks like a good simplification.

We can do that, but then the debugger would need to work a bit harder in order to get a constant, doesn't it? I don't feel strongly either way. If you do want it done the way you prefer, please let me know and I will make the change.

Anyway, this LGTM with the tests fixed.

These can be DIExpression, ConstantAsMetadata, etc, so Metadata looks to be the right specificity (and the underlying 'get' function it calls types them as Metadata too).

Perhaps the test fits in here https://github.com/llvm/llvm-project/blob/master/lldb/unittests/Expression/DWARFExpressionTest.cpp ?

Is this an NFC patch? If not, can you please add a test case?

Can you also add a unit test for this?

Thanks!

Cab you remind me why we are allowing ConstantAsMetadata nodes at all instead of allowing only DIExpressions?

Thanks!

I'm not sure if I specifically can help here: The only change I committed (on behalf of the author) was this one, which tests the echo "0x1" > %t.input case.

landed in 9b1c06c0e84a9cc763e12b289becb5fc3c9d01ea

This looks like a good start. What would you think about clang-formatting the tests using the LLVM style?

Address review feedback