[PATCH] D56587: Introduce DW_OP_LLVM_convert

Wed Feb 20 08:16:45 PST 2019

aprantl added inline comments.

================
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);
----------------
markus wrote:
> 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.
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?

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

https://reviews.llvm.org/D56587