- User Since
- Mar 2 2013, 8:12 AM (411 w, 2 d)
Tue, Jan 12
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.
Mon, Jan 11
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!
Fri, Jan 8
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).
Wed, Jan 6
Mechanically this LGTM.
Tue, Jan 5
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.
Dec 18 2020
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!
Dec 17 2020
Fix a potential cast error.
Dec 14 2020
This is needed because representation and specification (types of operands) of
it a bit different than actual dwarf expression DW_OP_implicit_pointer.
Can replaceDbgUsesWithUndef from Local.cpp be used here?
I think you accidentally deleted your patch :-)
Mechanically LGTM to me (one question inline).
Dec 11 2020
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?
Dec 10 2020
Mechanically this looks fine.
LGTM once all outstanding comments are addressed.
Dec 7 2020
Dec 4 2020
I think this looks reasonable.
Dec 2 2020
Nov 20 2020
This is fine apart from some nitpicks inside.
This is nice!
Nov 19 2020
Nov 17 2020
Oct 30 2020
Should we add __swift_ast, too?
Just FYI, we do have unit tests for the crashlog parser now, but I don't know how well this fits in there.
Oct 28 2020
Landed in 0b2b50a5d2893af47466f191771d20c9993a1624
Duncan, did you intentionally add yourself as a blocking reviewer or is this perhaps a misconfigured Herald rule?
Oct 27 2020
Also, I'm sorry for being so slow at reviewing these days...
Not an expert either, but at a high level this seems reasonable.
Oct 26 2020
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.
Oct 23 2020
Anyway, this LGTM with the tests fixed.
Oct 22 2020
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).
Oct 21 2020
Perhaps the test fits in here https://github.com/llvm/llvm-project/blob/master/lldb/unittests/Expression/DWARFExpressionTest.cpp ?
Oct 20 2020
Is this an NFC patch? If not, can you please add a test case?
Can you also add a unit test for this?
Oct 16 2020
Cab you remind me why we are allowing ConstantAsMetadata nodes at all instead of allowing only DIExpressions?
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?
Oct 15 2020
Address review feedback