[PATCH] D56587: Introduce DW_OP_LLVM_convert

Wed Feb 20 06:07:52 PST 2019

markus marked an inline comment as done.
markus added a comment.

In D56587#1402682 <https://reviews.llvm.org/D56587#1402682>, @dblaikie wrote:

> 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.

================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:516
+  uint64_t Offset = CU->ExprRefedBaseTypes[Index].Die->getOffset();
+  assert(Offset < (1ULL << (ULEB128PadSize * 7)) && "Offset wont fit");
+  AP->EmitULEB128(Offset, nullptr, ULEB128PadSize);
----------------
aprantl wrote:
> markus wrote:
> > markus wrote:
> > > aprantl wrote:
> > > > aprantl wrote:
> > > > > markus wrote:
> > > > > > markus wrote:
> > > > > > > aprantl wrote:
> > > > > > > > what happens when this assertion isn't met in a release compiler? Is that a purely hypothetical scenario?
> > > > > > > > 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.)
> > > > > > > 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. 
> > > > > > (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.
> > > > 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?
> > > >> (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 :)
> > > 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.
> 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.
> 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.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56587/new/

https://reviews.llvm.org/D56587