[PATCH] D56587: Introduce DW_OP_LLVM_convert

[Adrian Prantl via Phabricator via llvm-commits]

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);
----------------
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?


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

https://reviews.llvm.org/D56587