Page MenuHomePhabricator

llvm rejects DWARF operator DW_OP_lit[1-31] in IR
Needs ReviewPublic

Authored by alok on Apr 30 2020, 11:19 AM.

Details

Summary

Currently only DWARF operator DW_OP_lit0 is accepted other in variants like DW_OP_lit1-31 are rejected.

Summary

llvm doesnt accept DW_OP_lit[1-31], it accepts only DW_OP_lit0.
it produces below error.
. . . .
invalid expression
!DIExpression(53, 30, 159)
. . . .
Current patch completes support for DWARF operator DW_OP_lit[1-31].

Testing
  • Added unit test for validation thru llvm-dwarfdump
  • check-debuginfo
  • check-llvm

Diff Detail

Event Timeline

alok created this revision.Apr 30 2020, 11:19 AM

If I take your C input program and compile it to a .o file, I see that we do emit (DW_OP_lit13; DW_OP_lit0; DW_OP_mul; DW_OP_stack_value) for "var2".

I think what you are saying is, we do not accept DW_OP_litN as *input* in the IR. Is that correct? If so, please make sure the commit log states that.

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
400–402

I know you did not introduce this, so I won't insist you change it, but LLVM style says do not use else after a continue.

llvm/test/DebugInfo/dwarfdump-litN.ll
5

Remove the -mtriple, because you are using the %llc_dwarf substitution, which will add -mtriple when necessary.

15

Typo, "whether"

37

Remove the target triple line, so the input will work for any target.

I think this may actually be to some degree intentional. In LLVM IR. What's the use-case for this? Currently in LLVM IR a constant is always represented by a DW_OP_const(u) which makes the expressions easier to parse. LLVM knows how to *emit* DW_OP_litX, but I'm not sure what we gain from accepting it in LLVM IR.

alok marked 8 inline comments as done.May 1 2020, 1:48 AM

If I take your C input program and compile it to a .o file, I see that we do emit (DW_OP_lit13; DW_OP_lit0; DW_OP_mul; DW_OP_stack_value) for "var2".

I think what you are saying is, we do not accept DW_OP_litN as *input* in the IR. Is that correct? If so, please make sure the commit log states that.

Yes correct. The C program works because clang generates "DW_OP_constu, 5", which is optimized to DW_OP_lit5, but it doesnt accept DW_OP_lit5 in IR. I missed to state that clearly in commit log. I shall update the commit log. Thanks for your review.

I think this may actually be to some degree intentional. In LLVM IR. What's the use-case for this? Currently in LLVM IR a constant is always represented by a DW_OP_const(u) which makes the expressions easier to parse. LLVM knows how to *emit* DW_OP_litX, but I'm not sure what we gain from accepting it in LLVM IR.

Actually I found this while working on debugability support for flang. It looked strange that DW_OP_lit0 works but not other values. I understand that the same can be represented by DW_OP_const(u) but the smaller representation with DW_OP_lit is tempting. Supporting the same from completeness point of view as well as avoiding unnecessary issue reporting by the new front-ends emitting DW_OP_litX looked tempting.

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
400–402

Thanks for pointing this out, I shall correct this.

llvm/test/DebugInfo/dwarfdump-litN.ll
5

Thanks for your comment. I shall incorporate this.

15

Thanks for pointing this out. I shall incorporate this.

37

Thanks. I shall update this in next version.

alok marked 4 inline comments as done.May 1 2020, 1:49 AM

If we move forward with this, should we update the DIExpression::isConstant()?

alok updated this revision to Diff 261444.May 1 2020, 2:10 AM
alok retitled this revision from llvm rejects DWARF operator DW_OP_lit[1-31]. to llvm rejects DWARF operator DW_OP_lit[1-31] in IR.
alok edited the summary of this revision. (Show Details)

Updated to address comments from @probinson

If I take your C input program and compile it to a .o file, I see that we do emit (DW_OP_lit13; DW_OP_lit0; DW_OP_mul; DW_OP_stack_value) for "var2".

I think what you are saying is, we do not accept DW_OP_litN as *input* in the IR. Is that correct? If so, please make sure the commit log states that.

Yes correct. The C program works because clang generates "DW_OP_constu, 5", which is optimized to DW_OP_lit5, but it doesnt accept DW_OP_lit5 in IR. I missed to state that clearly in commit log. I shall update the commit log. Thanks for your review.

I think this may actually be to some degree intentional. In LLVM IR. What's the use-case for this? Currently in LLVM IR a constant is always represented by a DW_OP_const(u) which makes the expressions easier to parse. LLVM knows how to *emit* DW_OP_litX, but I'm not sure what we gain from accepting it in LLVM IR.

The inverse argument might be that keeping the LLVM IR in a more singular/canonical form will better for a semantic representation that's expected to be handled/mutated through the optimizations, then lowering to whatever is optimal at the end.

@aprantl - when this stuff was added, do you recall if there was any particular goal around that? Was lit0 added for some specific reason/separate from adding litN? Or was it just an oversight? And/or do you have thoughts on which way this should go today?

when this stuff was added, do you recall if there was any particular goal around that? Was lit0 added for some specific reason/separate from adding litN? Or was it just an oversight? And/or do you have thoughts on which way this should go today?

As I said yesterday, I think it would be better to aim for a canonical representation in LLVM to make transformations easier. A quick grep for DW_OP_lit0 in llvm/lit shows no current use of this, so we could remove it in favor of DW_OP_constu 0.
Doing some archeology, lit0 was added by @vsk in https://reviews.llvm.org/D48676 but that code must have been removed since. We could easily write a bitcode upgrade to canonicalize existing occurrences of lit0.

alok added a comment.May 2 2020, 7:04 AM

when this stuff was added, do you recall if there was any particular goal around that? Was lit0 added for some specific reason/separate from adding litN? Or was it just an oversight? And/or do you have thoughts on which way this should go today?

As I said yesterday, I think it would be better to aim for a canonical representation in LLVM to make transformations easier. A quick grep for DW_OP_lit0 in llvm/lit shows no current use of this, so we could remove it in favor of DW_OP_constu 0.
Doing some archeology, lit0 was added by @vsk in https://reviews.llvm.org/D48676 but that code must have been removed since. We could easily write a bitcode upgrade to canonicalize existing occurrences of lit0.

Should we do upgrade to other lits (1-31) also in place of rejecting them ?

when this stuff was added, do you recall if there was any particular goal around that? Was lit0 added for some specific reason/separate from adding litN? Or was it just an oversight? And/or do you have thoughts on which way this should go today?

As I said yesterday, I think it would be better to aim for a canonical representation in LLVM to make transformations easier. A quick grep for DW_OP_lit0 in llvm/lit shows no current use of this, so we could remove it in favor of DW_OP_constu 0.
Doing some archeology, lit0 was added by @vsk in https://reviews.llvm.org/D48676 but that code must have been removed since. We could easily write a bitcode upgrade to canonicalize existing occurrences of lit0.

Should we do upgrade to other lits (1-31) also in place of rejecting them ?

I'm assuming that once we have the code for upgrading lit0, handling the other lit operators will not really add any complexity, so I'm fine either way, if that makes things easier.

when this stuff was added, do you recall if there was any particular goal around that? Was lit0 added for some specific reason/separate from adding litN? Or was it just an oversight? And/or do you have thoughts on which way this should go today?

As I said yesterday, I think it would be better to aim for a canonical representation in LLVM to make transformations easier. A quick grep for DW_OP_lit0 in llvm/lit shows no current use of this, so we could remove it in favor of DW_OP_constu 0.
Doing some archeology, lit0 was added by @vsk in https://reviews.llvm.org/D48676 but that code must have been removed since. We could easily write a bitcode upgrade to canonicalize existing occurrences of lit0.

Should we do upgrade to other lits (1-31) also in place of rejecting them ?

I'm assuming that once we have the code for upgrading lit0, handling the other lit operators will not really add any complexity, so I'm fine either way, if that makes things easier.

I'd be a little against it - adding compatibility for something we were never compatible with in the first place - but not strongly enough to make/break it.

Orlando added a subscriber: Orlando.May 5 2020, 2:23 AM

when this stuff was added, do you recall if there was any particular goal around that? Was lit0 added for some specific reason/separate from adding litN? Or was it just an oversight? And/or do you have thoughts on which way this should go today?

As I said yesterday, I think it would be better to aim for a canonical representation in LLVM to make transformations easier. A quick grep for DW_OP_lit0 in llvm/lit shows no current use of this, so we could remove it in favor of DW_OP_constu 0.
Doing some archeology, lit0 was added by @vsk in https://reviews.llvm.org/D48676 but that code must have been removed since. We could easily write a bitcode upgrade to canonicalize existing occurrences of lit0.

Should we do upgrade to other lits (1-31) also in place of rejecting them ?

I'm assuming that once we have the code for upgrading lit0, handling the other lit operators will not really add any complexity, so I'm fine either way, if that makes things easier.

I'd be a little against it - adding compatibility for something we were never compatible with in the first place - but not strongly enough to make/break it.

+1 a little against it - I don't see the appeal of moving the DIExpressions any closer to the final (DWARF) output. It's more on principle than against this specific change though; As @aprantl pointed out this case will be trivial to handle. But on the other hand, the lit0 case which already exists shows that setting a precedent could be significant, IMO.