Page MenuHomePhabricator

Introduce DW_OP_LLVM_convert
ClosedPublic

Authored by markus on Jan 11 2019, 4:04 AM.

Details

Summary

Introduce a DW_OP_LLVM_convert Dwarf expression pseudo op that allows for a convenient way to perform type conversions on the Dwarf expression stack. As an additional bonus it paves the way for using other Dwarf v5 ops that need to reference a base_type.

  • Use the new operation in llvm::replaceAllDbgUsesWith where it replaces a complex (and broken) shift and mask based expression to perform sign- and zero-extension.
  • Update the AsmPrinter parts to allow for referencing a base_type from Dwarf expressions.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I don't think anyone mentioned it, that's why I was bringing it up - there's always an option to not render any location if it's not possible/worth the work. That's all I was asking - is it worth the complexity? (I wasn't sure anyone needed it - but sounds like Sony does, reckon it's worth the tradeoff in complexity in LLVM compared to the work required to support this in the Sony debugger?)

NVPTX also would need it, because they are stuck on DWARF v2.

I don't think anyone mentioned it, that's why I was bringing it up - there's always an option to not render any location if it's not possible/worth the work. That's all I was asking - is it worth the complexity? (I wasn't sure anyone needed it - but sounds like Sony does, reckon it's worth the tradeoff in complexity in LLVM compared to the work required to support this in the Sony debugger?)

NVPTX also would need it, because they are stuck on DWARF v2.

Any ideas if NVPTX hit this case? my understanding was that NVPTX has a fairly restrictive set of code or actions that can be used.

markus updated this revision to Diff 186431.Feb 12 2019, 2:47 AM

Removed Label from DIE (added DenseMap to CU and pass it around). Rebased to top of trunk.

aprantl added inline comments.Feb 12 2019, 9:21 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1938

This is to inject the reference to the basic type die, right?

This code should probably be factored out into a relocate/fixupTypeRefs() helper function. I also assume that you need to apply the same fixup for the case of a single, non-debug_lo, inline DW_AT_location, right?

The fact that the placeholder is encoded as a LEB128 sounds really dangerous. If we ever support any branching operations, it will mess with the offsets. Can we assume that the finalized DIE ref will always be a DW_OP_ref_addr or something with a fixed size? Could we make the placeholder the same fixed size, too?

If that doesn't work, the right solution is probably to defer the emission of DwarfExpressions until here, which we could do in a separate, preparatory commit.

markus marked an inline comment as done.Feb 12 2019, 11:49 AM
markus added inline comments.
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1938

This is to inject the reference to the basic type die, right?

Yes

This code should probably be factored out into a relocate/fixupTypeRefs() helper function. I also assume that you need to apply the same fixup for the case of a single, non-debug_lo, inline DW_AT_location, right?

Sounds reasonable. Not sure I could easily find where in the code the inline expressions are inserted though. If you could point at a file and line number that would be helpful. I guess another option would be to force these expressions (the ones containing a base type reference) to always end up in .debug_loc right?

The fact that the placeholder is encoded as a LEB128 sounds really dangerous. If we ever support any branching operations, it will mess with the offsets. Can we assume that the finalized DIE ref will always be a DW_OP_ref_addr or something with a fixed size? Could we make the placeholder the same fixed size, too?

The spec states that the finalized base type DIE offset is encoded as a ULEB128 so not much choice about that but the value we pick up here (the one inserted in DwarfExpression::addExpression) is just a index so we could certainly encode that in a fixed size integer. If branches were to be introduced at a later point I imagine that the branch target in the emitted dwarf would be a label (MCSymbol) but in the intermediate expression vector a simple offset would probably suffice.

If that doesn't work, the right solution is probably to defer the emission of DwarfExpressions until here, which we could do in a separate, preparatory commit.

I think that would be a good thing to do but unfortunately it seems far from easy to get rid of the stuff that is in between.

aprantl added inline comments.Feb 12 2019, 12:46 PM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1938

If you could point at a file and line number that would be helpful.

git grep addBlock.*DW_AT_location lib/CodeGen/AsmPrinter
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:    addBlock(*VariableDIE, dwarf::DW_AT_location, DwarfExpr->finalize());
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:        addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize());
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:  addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize());
lib/CodeGen/AsmPrinter/DwarfUnit.cpp:        addBlock(ParamDIE, dwarf::DW_AT_location, Loc);

The spec states that the finalized base type DIE offset is encoded as a ULEB128 so not much choice about that but the value we pick up here (the one inserted in DwarfExpression::addExpression) is just a index so we could certainly encode that in a fixed size integer.

I see. I didn't think about emitting branch targets a label differences, I thought we'd just hardcode the offsets. I guess we can defer this until it becomes an issue. Encoding the temporary die reference with a fixed size would probably still be a good idea, just to keep this code simpler.

probinson added inline comments.Feb 12 2019, 1:02 PM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1938

Re the ULEB as how to find the base_type DIEs, the unstated assumption is that the base_type DIEs would be emitted unconditionally at the top of the CU, so everyone can just use them as needed. If you want to emit base_types lazily... don't do that.
Re branches, there are already branch operators, if that's what you're talking about (DW_OP_skip and _bra).

dblaikie added inline comments.Feb 12 2019, 1:36 PM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1938

I think maybe you don't need to make that assumption - you can produce a label difference as a uleb (though that may get complicated).

Yeah, we use it in debug_rnglists, for instance:

.uleb128 .Lfunc_end0-.Lfunc_begin0 #   length

& even if I change that to be a label difference between labels that bound the uleb itself (ie: where the difference would vary depending on the number of bytes required for the uleb) clang still assembles it at least... :)

markus marked 2 inline comments as done.Feb 13 2019, 12:29 AM
markus added inline comments.
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1938

Re the ULEB as how to find the base_type DIEs, the unstated assumption is that the base_type DIEs would be emitted unconditionally at the top of the CU, so everyone can just use them as needed. If you want to emit base_types lazily... don't do that.

I played with emitting the base_type DIEs directly after the CU header but since the size of that header varies and due to the phase ordering of how the debug info is emitted I still need label differences to be able to locate the base_type DIEs in a robust manner. Right now I would not say that they are emitted lazily but rather we find out which ones we need and then emit them in table form. Still need the labels though.

Re branches, there are already branch operators, if that's what you're talking about (DW_OP_skip and _bra).

Where do you see these branch operators being used? I can't find them.

1938

I think maybe you don't need to make that assumption - you can produce a label difference as a uleb (though that may get complicated).

Yes, that is what I currently do. It looks like this

.byte   168                     # DW_OP_convert
.uleb128 .Lbase_type0-.Lcu_begin0

& even if I change that to be a label difference between labels that bound the uleb itself (ie: where the difference would vary depending on the number of bytes required for the uleb) clang still assembles it at least... :)

Yep, I thought about that too when I realized that I need to add some prototype support for our downstream assembler. Came to the conclusion that I could treat the ULEB128s as fixed size by sign/zero extending them, so that should simplify things a lot even though it is not space efficient... Either way it is not a problem for this review how the assembler solves it :)

+ @ABataev re the question whether NVPTX runs into the situation described in this review.

The Sony debugger guys are okay with using the GCC operator in a pre-v5 expression. So, tentatively, for all debugger tunings, we can emit that instead of the more complicated expression. That way we are emitting compliant expressions, and the info doesn't just disappear sometimes (a much worse outcome IMO). The only remaining question is my hypothetical about NVPTX.

Re branch operators, I thought Adrian was throwing that out there as a general concern; yes branch operators exist, and yes we don't use them currently. As David says, the assembler knows how to convert a label difference into a ULEB and it will all Just Work. If/when we ever need it to.

I see now that the inlined places i.e. these:

git grep addBlock.*DW_AT_location lib/CodeGen/AsmPrinter
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:    addBlock(*VariableDIE, dwarf::DW_AT_location, DwarfExpr->finalize());
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:        addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize());
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:  addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize());
lib/CodeGen/AsmPrinter/DwarfUnit.cpp:        addBlock(ParamDIE, dwarf::DW_AT_location, Loc);

work quite differently and that will need some additional work.

The good news is that here it seems we might just have the infrastructure (DIEValue etc ) to realize the base_type reference in a much cleaner way.

+ @ABataev re the question whether NVPTX runs into the situation described in this review.

The Sony debugger guys are okay with using the GCC operator in a pre-v5 expression. So, tentatively, for all debugger tunings, we can emit that instead of the more complicated expression. That way we are emitting compliant expressions, and the info doesn't just disappear sometimes (a much worse outcome IMO). The only remaining question is my hypothetical about NVPTX.

Re branch operators, I thought Adrian was throwing that out there as a general concern; yes branch operators exist, and yes we don't use them currently. As David says, the assembler knows how to convert a label difference into a ULEB and it will all Just Work. If/when we ever need it to.

NVPTX supports only DWARF2 and does not know anything about DWARF5 operations. Also, it does not support any type of the expression in the DWARF sections, except for <section_name>+-<int_offset>.

NVPTX supports only DWARF2 and does not know anything about DWARF5 operations. Also, it does not support any type of the expression in the DWARF sections, except for <section_name>+-<int_offset>.

Isn't the latter a rather restrictive limitation that should be addressed in the NVPTX assembler?

NVPTX supports only DWARF2 and does not know anything about DWARF5 operations. Also, it does not support any type of the expression in the DWARF sections, except for <section_name>+-<int_offset>.

Isn't the latter a rather restrictive limitation that should be addressed in the NVPTX assembler?

I agree, but this the limitation we have to live with, unfortunately. Either we support that limitation to have the debug info for NVPTX, or we don't have debug info for NVPTX. I cannot make NVidia to update their ptxas tool to support complex expressions in DWARF sections.

markus updated this revision to Diff 187015.Feb 15 2019, 7:36 AM

Addressed the inlined location expressions.

Got rid of all the label differences introduced in previous patches and instead use padded ULEB128s of a fixed size (4 bytes).

Using fixed size allows us to leverage the existing framework much better and avoid spraying symbols all over to place just to be able to cope with the variable sized .uleb128 directives. One drawback is that debug output is perhaps slightly less space efficient but that is only a matter of a few bytes each time a base_type is referenced from a DW_OP_convert.

Comments please (I know that I need to add lit tests and clean up comments).

markus updated this revision to Diff 187203.Feb 18 2019, 2:34 AM
markus edited the summary of this revision. (Show Details)
  • Did some cleanup
  • Added tests
  • Emit DW_OP_convert for Dwarf5
  • Emit legacy shift and mask expression for Dwarf4 and lower
  • Added llc option -generate-typed-dwarf5-expr to force emission regardless of targeted Dwarf version
aprantl added inline comments.Feb 18 2019, 10:12 AM
lib/CodeGen/AsmPrinter/DIE.cpp
512

under the current style you may drop the duplicate doxygen comments con the function implementations.

516

what happens when this assertion isn't met in a release compiler? Is that a purely hypothetical scenario?

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
1197

for (auto &I : reverse(ExprRefedBaseTypes))

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
396

Why not use a SmallDenseSet for CU.ExprRefedBaseTypes?

test/DebugInfo/Generic/convert-inlined.ll
32 ↗(On Diff #187203)

Sorry about the extra work this causes, but it would really pay off to also have a separate peraratory commit that adds dwarfdump support for DW_OP_convert so this is printed as DW_OP_convert (0x000022 "DW_ATE_signed_32") as well as dwarfdump --verify support that ensures that the offset actually points to a type DIE.

markus marked 3 inline comments as done.Feb 19 2019, 1:34 AM
markus added inline comments.
lib/CodeGen/AsmPrinter/DIE.cpp
516

what happens when this assertion isn't met in a release compiler?

The debug info would be corrupted.

Is that a purely hypothetical scenario?

With ULEB128PadSize = 4 we are fine as long as the types are placed within 256MB from the start of the .debug_info section. Since we take care to insert the types immediately after the CU this will not be an issue for the case of a single CU. However by using llvm-link it is possible to put several CUs in the same module and that could push us closer to the 256MB limit.

If we set ULEB128PadSize = 5 then the limit becomes 32GB and that should put us on the safe side for a considerable future (keeping in mind that this limit is for object files and not the final linked executable).

(One could argue that symbols and label differences, effectively pushing the problem to the assembler, are the right way to go here and I would tend to agree with that but unfortunately that causes havoc in the existing DIE framework as it relies on being able to pre-compute sizes.)

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
396

I don't think that would work as we rely on being able to index into it.

test/DebugInfo/Generic/convert-inlined.ll
32 ↗(On Diff #187203)

I could look into that but before I spend too much time on dwarfdump I would like to get confirmation that would be the last remaining issue.

markus marked an inline comment as done.Feb 19 2019, 2:28 AM
markus added inline comments.
lib/CodeGen/AsmPrinter/DIE.cpp
516

However by using llvm-link it is possible to put several CUs in the same module and that could push us closer to the 256MB limit.

Actually forget that part. I see now that is irrelevant since the offset is within CU and not from start of .debug_info. I got confused looking at the output of readelf as that tool prints .debug_info address even though it is encoded as a CU offset.

So given this I think the 256MB limit is perfectly reasonable.

aprantl added inline comments.
lib/CodeGen/AsmPrinter/DIE.cpp
516

(One could argue that symbols and label differences, effectively pushing the problem to the assembler, are the right way to go here and I would tend to agree with that but unfortunately that causes havoc in the existing DIE framework as it relies on being able to pre-compute sizes.)

@dblaikie has dealt with similar problems in the past while refactoring AsmPrinter to support DWOs, perhaps he has some ideas?

So given this I think the 256MB limit is perfectly reasonable.

A good measure to decide this is to look at the size of an LTO build of Clang with all targets, which is usually a good proxy for what we can expect from large programs.

aprantl added inline comments.Feb 19 2019, 8:51 AM
lib/CodeGen/AsmPrinter/DIE.cpp
516

When you say

causes havoc in the existing DIE framework as it relies on being able to pre-compute sizes

is the problem that we don't know the offsets of the location list entries ahead of time since they depend on the position of the referenced type DIEs and thus we don't know the size of the DW_AT_location attributes, or is the problem only within the location list section?

markus marked 2 inline comments as done.Feb 19 2019, 10:24 AM
markus added inline comments.
lib/CodeGen/AsmPrinter/DIE.cpp
516

(One could argue that symbols and label differences, effectively pushing the problem to the assembler, are the right way to go here and I would tend to agree with that but unfortunately that causes havoc in the existing DIE framework as it relies on being able to pre-compute sizes.)

@dblaikie has dealt with similar problems in the past while refactoring AsmPrinter to support DWOs, perhaps he has some ideas?

While I do think that solving this with label differences would be the right thing to do in general I do not think it is the right thing to do here as the changes become much larger than they need to be.

So given this I think the 256MB limit is perfectly reasonable.

A good measure to decide this is to look at the size of an LTO build of Clang with all targets, which is usually a good proxy for what we can expect from large programs.

Yes, to clarify this. Since the base types that we generate are inserted immediately after the CU DIE we would need to insert a huge amount of these (>16 million I believe) to hit the 256MB limit. This should be quite impossible since we don't create entries for duplicate types and there simply aren't that many unique ones :)

516

is the problem that we don't know the offsets of the location list entries ahead of time since they depend on the position of the referenced type DIEs and thus we don't know the size of the DW_AT_location attributes, or is the problem only within the location list section?

One of the problems is that as soon as we emit a .uleb128 directive of a label difference we do not know that size of that and hence cannot compute the size of the block it is in without emitting more labels (begin and end for that block) and then do a label difference of that too. This is not how the DIE handling is currently designed (IIUC) as it relies on being able to compute the size of each DIE. This is especially a problem for the inlined DW_AT_location attributes.

This is why having padded / fixed size ULEB128s, as we have in the current patch, makes things much easier.

aprantl added inline comments.Feb 19 2019, 10:34 AM
lib/CodeGen/AsmPrinter/DIE.cpp
516

So we only insert the special type DIEs into the very first DW_TAG_compile_unit? Then this is fine. It wasn't clear to me whether in the LTO case we'd inject the types into every CU. There's no need to do that, so if we don't then.we're good.

I'm a touch confused by the whole discussion here - so I'll write some things & perhaps folks can correct my understanding where necessary.

The issue is that a location expression (either in a direct location, or in a loclist) needs to reference a DIE.

Sounds like that DIE reference is necessarily CU-local (because we're talking about precomputing the offset - and we could only do that if it's CU-local).

We already emit other CU-local DIE references in attributes (eg: DW_AT_specification, etc) references with hardcoded 4 bytes in size- so why would it be problematic to emit this one in the same way (with a padded ULEB128 that we know will give us 4 bytes of offset to work with)?

& yeah, maybe if someone wants to save us some size (at the cost of whatever computational complexity this invokes in the assembler) it could be replaced with label differences (doing label differences for all DIE references would be nifty for manual editing/mucking about with DWARF, but not a big deal).

markus marked an inline comment as done.Feb 20 2019, 6:06 AM

I'm a touch confused by the whole discussion here - so I'll write some things & perhaps folks can correct my understanding where necessary.

I think that we all are. Maybe it is because we tried some many different approaches in the same review that it is a bit convoluted now.

The issue is that a location expression (either in a direct location, or in a loclist) needs to reference a DIE.

Correct.

Sounds like that DIE reference is necessarily CU-local (because we're talking about precomputing the offset - and we could only do that if it's CU-local).

Also correct. The spec says that the reference is an offset into the current CU.

We already emit other CU-local DIE references in attributes (eg: DW_AT_specification, etc) references with hardcoded 4 bytes in size- so why would it be problematic to emit this one in the same way (with a padded ULEB128 that we know will give us 4 bytes of offset to work with)?

Agree. It should be no more problematic than what is done in other places already. The 4 byte ULEB128 gives us 28 bits to encode the offset in but as reasoned in other comments there is no way that limit can be reached.

& yeah, maybe if someone wants to save us some size (at the cost of whatever computational complexity this invokes in the assembler) it could be replaced with label differences (doing label differences for all DIE references would be nifty for manual editing/mucking about with DWARF, but not a big deal).

Agree.

lib/CodeGen/AsmPrinter/DIE.cpp
516

So we only insert the special type DIEs into the very first DW_TAG_compile_unit? Then this is fine. It wasn't clear to me whether in the LTO case we'd inject the types into every CU. There's no need to do that, so if we don't then.we're good.

The special type DIEs are local to the CU that use them so they will not necessarily only be inserted into the first one but rather into the ones where they are used. This is not a problem though since according to the spec 'the operand is an unsigned LEB128 number that represents the offset of a debugging information entry in the current compilation unit' i.e. the offset is relative to the current CU so there is no chance that it will exceed the 256MB limit since they are inserted immediately after the DW_TAG_compile_unit DIE.

I have created a review for the requested 'llvm-dwarfdump' updates (the printing part) in https://reviews.llvm.org/D58442

markus updated this revision to Diff 187577.Feb 20 2019, 7:05 AM

+ Addressed review remark (use range-based loop instead of iterators).
+ Updated tests (probably forgot during last rebase. Verified the values used now with a clean master)

aprantl added inline comments.Feb 20 2019, 8:16 AM
lib/CodeGen/AsmPrinter/DIE.cpp
516

If they are CU-relative I agree that should be perfectly safe, thanks.

What happens if I llvm-link two CUs that both contain the same DIExpression. Is it possible for two location list entries to be uniques across compile units? I think the answer is no, right?

markus marked an inline comment as done.Feb 20 2019, 11:21 AM
markus added inline comments.
lib/CodeGen/AsmPrinter/DIE.cpp
516

What happens if I llvm-link two CUs that both contain the same DIExpression. Is it possible for two location list entries to be uniques across compile units? I think the answer is no, right?

Correct. The data structure is per CU so nothing happens across such units.

markus updated this revision to Diff 187774.Feb 21 2019, 5:50 AM
  • updated to top of trunk to have the llvm-dwarfdump updates committed earlier today
  • updated tests and made additional improvements to llvm-dwarfdump
  • fixed segfault bug where we crashed on release builds but not debug builds (the allocation of DIEBaseTypeRef)

Are we good to land this now or what is remaining?

There's a nonzero chance that this patch will break llvm/tools/dsymutil. I'll see if I can come up with a testcase for that (hopefully later today).

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
1195

... so their offsets fit into the 5 bits reserved inside the location expressions.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1937

You might want to reword this comment to be more assertive :-)

Is my understanding correct that we only need to do this here because the inline DW_AT_location (DW_FORM_block, ..) are emitted ahead of time and thus have the correct offsets injected from the get go?

Could you please move this out into a fixupLocEntryDIEReferences() (or something) function?

1977

Didn't your dwarfdump patch have this info in an enum or am I confusing things?

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
29

Why is this needed? Shouldn't we key off the debugger tuning flag?

395

Can you add a comment that explains what happens in the non-location-list cases?

probinson added inline comments.Feb 21 2019, 11:31 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
948

A separate CU for basic types would not be usable by DW_OP_convert? because it uses CU-relative offsets to find them.

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
29

The debugger-tuning design is that we use it only in the DwarfDebug ctor to set other control flags, which can all be influenced independently. Tuning and its associated flags are not per-CU, although DWARF version is.

392

Pre-v5 needs to emit the GNU op, not the standard op.

Could you please add a third testcase with two DICompileUnits, both of which have a DIExpression with a DW_OP_LLVM_convert that will use the same type and then verify that we emit the type in both CUs? This will help guard against later modifications breaking this scenario.

Thanks!

test/DebugInfo/Generic/convert-debugloc.ll
144 ↗(On Diff #187774)

I assume most of these attributes aren't needed?

markus updated this revision to Diff 187926.Feb 22 2019, 4:52 AM
  • Added test with two CUs.
  • Updated some comments.
  • Removed the -generate-typed-dwarf5-expr option, will only produce DW_OP_convert for Dwarf v5, for all prior versions the mask & shift expression is used. I suggest that we forget about DW_OP_GNU_convert and debugger tunings for this review. If that is really desired it can be added later in a separate patch.
markus marked 6 inline comments as done.Feb 22 2019, 5:07 AM
markus added inline comments.
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1937

Is my understanding correct that we only need to do this here because the inline DW_AT_location (DW_FORM_block, ..) are emitted ahead of time and thus have the correct offsets injected from the get go?

Sort of. When these are inserted in DwarfExpression::addExpression (for both location-lists and inlined) the offset of the base_type DIE is not known so we need to insert a placeholder. For the the location-list case the data structure is unfortunately a plain byte stream so we need this elaborate state machine to extract the placeholder here.

Could you please move this out into a fixupLocEntryDIEReferences() (or something) function?

Since this is a state machine and hence keeps state I think that putting it in a separate function would only make it messier.

1977

DWARFExpression does contain similar information but it is private there. Not sure if refactoring would be worthwhile.

I don't think anyone mentioned it, that's why I was bringing it up - there's always an option to not render any location if it's not possible/worth the work. That's all I was asking - is it worth the complexity? (I wasn't sure anyone needed it - but sounds like Sony does, reckon it's worth the tradeoff in complexity in LLVM compared to the work required to support this in the Sony debugger?)

NVPTX also would need it, because they are stuck on DWARF v2.

Any ideas if NVPTX hit this case? my understanding was that NVPTX has a fairly restrictive set of code or actions that can be used.

Ping on this - still wondering if anyone needs the complicated code or if we could get away with the GNU extension + DWARFv5 standard form.

I don't think anyone mentioned it, that's why I was bringing it up - there's always an option to not render any location if it's not possible/worth the work. That's all I was asking - is it worth the complexity? (I wasn't sure anyone needed it - but sounds like Sony does, reckon it's worth the tradeoff in complexity in LLVM compared to the work required to support this in the Sony debugger?)

NVPTX also would need it, because they are stuck on DWARF v2.

Any ideas if NVPTX hit this case? my understanding was that NVPTX has a fairly restrictive set of code or actions that can be used.

Ping on this - still wondering if anyone needs the complicated code or if we could get away with the GNU extension + DWARFv5 standard

not sure, if NVPTX can hit this. It has no stack at all, so, maybe, this op won't be generated at all

markus updated this revision to Diff 188533.Feb 27 2019, 7:43 AM
  • Rewrote the expression parsing state machine in DwarfDebug::emitDebugLocEntry to use DWARFExpression as this avoids duplication of code describing ops and their arguments (got inspired by the dsymutil review).
aprantl added inline comments.Feb 27 2019, 9:14 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1944

Nice. Should we do the same thing as in dsymutil here and check for Encoding::BaseTypeRef?

markus updated this revision to Diff 188710.Feb 28 2019, 4:43 AM
  • Removed explicit check for DW_OP_convert and replaced with a generic handling of operands that should work with all types (regardless if it is first, second or both that is BaseTypeRef).

We're almost there!

lib/CodeGen/AsmPrinter/DIE.cpp
512

please remove this comment from the implementation.

521

ditto.. it should be on the declaration in the header file.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1931

Remove the first word.

1944

Could you please still factor this into a static fixupBaseTypeRefs(or something along those lines) function for better readability?

1947

Please add an assert that fails if the opcode is DW_OP_const_type as it is not supported by this loop.

1949

Where is this getting copied?

markus marked 2 inline comments as done.Mar 1 2019, 12:48 AM

We're almost there!

Yay :)

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1944

Sure, but I don't understand where to place the cut to improve readability. I.e. what should go into fixupBaseTypeRefs and what should remain in emitDebugLocEntry?

1949

It is not getting copied at all, 'Encoding::SizeNA` indicates that the operation does not have an operand in this slot. Or maybe I am not understanding the question?

aprantl accepted this revision.Mar 1 2019, 8:12 AM
aprantl added inline comments.
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1949

My bad, I was thinking about a non-base-type operand, but it is actually handled in the else branch!

1949

On second thought, I think we can leave it as is, too.

This revision is now accepted and ready to land.Mar 1 2019, 8:12 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2019, 1:48 AM
Herald added a subscriber: ormris. · View Herald Transcript
ormris removed a subscriber: ormris.Mar 19 2019, 8:54 AM