[PATCH] D26769: [IR] Remove the DIExpression field from DIGlobalVariable.

[Adrian Prantl via llvm-commits]

> On Nov 18, 2016, at 9:55 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> No comment on whether we need an offset/etc.; but assuming we do want that some day, it might be more efficient to add an offset field to DIGlobalVariableExpr:
> 
> DIGlobalVariableExpr(value: DW_OP_const 42, offset: 32 bits) -> DIGlobalVariable(name: foo)

I'm very much against this. We probably want to share as much infrastructure as possible between global and local variables. Putting special support into DIGlobalVariableExpresssion moves us farther away from that goal.

With DIExpression (which is really a DWARF expression) we have an extensible representation for all sorts of operations that may be necessary to reconstruct a source variable. I'm fine with improving the syntax to make it more readable, but I would not want to make it any less generic.

-- adrian

> 
> 
>> On 2016-Nov-18, at 09:51, David Blaikie <dblaikie at gmail.com> wrote:
>> 
>> Would it be reasonable/useful to add another level of indirection while we're here - that would describe which part of the variable is being described by this location:
>> 
>> DIGlobalVariableExpr(value: DW_OP_const 42) -> DIGlobalVariableFragment(offset: 32 bits) -> DIGlobalVariable(name: foo)
>> 
>> (just guessing/pretending there's a DW_OP_const for constant values - I forget how it all works, but purely for demonstration purposes)
>> 
>> That way code manipulating DIGlobalVariableExprs wouldn't need to have explicit knowledge of which part of the variable is being described here. (I suppose that could end up with us creating a bottom-up expression tree in metadata, which probably isn't ideal (too heavyweight))
>> 
>> Perhaps keeping the bit_piece parts out of the DWARF expression bytes and in a separate field of the DIGlobalVariableExpr?
>> 
>> DIGlobalVariableExpr(offset: 32, value: DW_OP_const 42)
>> 
>> Then at least when a variable moves around (does that happen? maybe? - I suppose something like asan, smooshing everything into a single alloca would move something around without splitting it up) the expression can be updated relatively simply without unpacking through a DW_OP_bit_piece, etc?
>> 
>> Maybe this doesn't matter - Ih aven't looked at much of the expression/location handling code.
>> 
>> On Fri, Nov 18, 2016 at 9:32 AM Adrian Prantl <aprantl at apple.com> wrote:
>> aprantl removed rL LLVM as the repository for this revision.
>> aprantl updated this revision to Diff 78544.
>> aprantl added a comment.
>> 
>> Improved type-safety by adding a DIGlobalVarExpr pointer union.
>> 
>> 
>> https://reviews.llvm.org/D26769
>> 
>> Files:
>>  include/llvm/Bitcode/LLVMBitCodes.h
>>  include/llvm/IR/DIBuilder.h
>>  include/llvm/IR/DebugInfo.h
>>  include/llvm/IR/DebugInfoMetadata.h
>>  include/llvm/IR/GlobalVariable.h
>>  include/llvm/IR/Metadata.def
>>  lib/Analysis/ModuleDebugInfoPrinter.cpp
>>  lib/AsmParser/LLParser.cpp
>>  lib/Bitcode/Reader/BitcodeReader.cpp
>>  lib/Bitcode/Writer/BitcodeWriter.cpp
>>  lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
>>  lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
>>  lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
>>  lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>  lib/IR/AsmWriter.cpp
>>  lib/IR/DIBuilder.cpp
>>  lib/IR/DebugInfo.cpp
>>  lib/IR/DebugInfoMetadata.cpp
>>  lib/IR/LLVMContextImpl.h
>>  lib/IR/Metadata.cpp
>>  lib/IR/Verifier.cpp
>>  lib/Transforms/IPO/StripSymbols.cpp
>>  lib/Transforms/Instrumentation/AddressSanitizer.cpp
>>  test/Assembler/diglobalvariable.ll
>>  test/Assembler/diglobalvariableexpression.ll
>>  test/Bitcode/DIGlobalVariableExpr.ll
>>  test/Bitcode/DIGlobalVariableExpr.ll.bc
>>  test/Bitcode/diglobalvariable-3.8.ll
>>  test/Bitcode/diglobalvariable-3.8.ll.bc
>>  test/DebugInfo/X86/multiple-at-const-val.ll
>>  test/DebugInfo/X86/pr12831.ll
>>  test/DebugInfo/X86/split-global.ll
>>  test/DebugInfo/X86/stack-value-dwarf4.ll
>>  test/DebugInfo/X86/unattached-global.ll
>>  test/Transforms/GlobalMerge/debug-info.ll
>>  unittests/IR/MetadataTest.cpp
>> 
>