Debug info for variables whose type is shrinked to bool

[NikolaPrica]

Summary

During the globalopt transformation, when shrinking variable type into boolean, debug information for static global variables is dispatched. As a result there is no DW_AT_location and the debugger is not able to read such variables.

[aprantl]

lib/Transforms/IPO/GlobalOpt.cpp

1577 ↗

(On Diff #108641)

I'm not familiar with this transformation: Do we need to add a DIExpression to mask out all but the first bit (i.e. can multiple bools be packed into the same uint32_t such that it could confuse debuggers)?

test/Transforms/GlobalOpt/static-global-boolean-dwarf.c

2 ↗

(On Diff #108641)

We shouldn't require a frontend to be available when running tests. Can you use thew output of clang -S -emit-llvm instead an run it through opt?

[NikolaPrica]

lib/Transforms/IPO/GlobalOpt.cpp

1577 ↗

(On Diff #108641)

The debug info which is provided here with addDebugInfo later generates address of the variable (DW_OP_addr: xxxx) in DW_AT_location. If we provide here metadata which is for 1byte variable the debugger would get confused because the enum type is written as 4-byte and he would try to read 4 bytes. This is just temporary fix until proper way to handle this is found.

[NikolaPrica]

Changing test file from .c to .ll

[NikolaPrica]

Sorry for spaming. First time using Phabricator. Uploaded wrong diff.

[probinson]

test/Transforms/GlobalOpt/static-global-boolean-dwarf.ll

2 ↗

(On Diff #109057)

It would be more usual to pipe the output of opt directly to llvm-dwarfdump, unless opt can't do that.
(It helps the tests run faster to avoid writing files unless you really have to.)

128 ↗

(On Diff #109057)

These checks verify that something named "foo" has debug info of some kind, but without verifying any details. I'd rather see verification of the location expression, and possibly the type, given that you are planning to change those in the future to describe the shrunk variable.

[aprantl]

lib/Transforms/IPO/GlobalOpt.cpp

1577 ↗

(On Diff #108641)

If I understood you correctly then the best way to represent this would be a DW_OP_LLVM_fragment /*offset*/0 /*bitsize*/1 (or 8?)
expression. This will get lowered into a DW_OP_bit_piece to tell the debugger that this location is describing of the first n bits of the variable.

[NikolaPrica]

This patch implements new DIExpression :

 val * (ValOther - ValInit) + ValInit:
 DW_OP_deref DW_OP_constu <ValMinus>
DW_OP_mul DW_OP_constu <ValInit> DW_OP_plus DW_OP_stack_value

The other expression which was suggested was :

// val * 42 | ((~val)&1) * 87
DW_OP_dup DW_OP_deref DW_OP_constu 42 DW_OP_mul DW_OP_swap
DW_OP_deref DW_OP_not DW_OP_lit1 DW_OP_and DW_OP_constu 87 DW_OP_mul
DW_OP_or DW_OP_stack_value

I have chosen to implement first because it is simpler.
I have also added simpler test also with concrete DW_AT_location description .

[aprantl]

In D35994#838113, @NikolaPrica wrote:

This patch implements new DIExpression :

val * (ValOther - ValInit) + ValInit:

If we are doing this expression, there should be an assertion in the code that ValInit < ValOther.

 DW_OP_deref DW_OP_constu <ValMinus>
DW_OP_mul DW_OP_constu <ValInit> DW_OP_plus DW_OP_stack_value

Why does Val need to be dereferenced?

If we use our concrete example with 42 and 87 again:

start: <val> // either 0 or 1
DW_OP_deref // not sure why this is needed, but let's assume we still have 0 or 1 on the stack
DW_OP_constu (87-42) // <0 or 1> <45>
DW_OP_mul // <0 or 45>
DW_OP_constu 42 // <0 or 45> <42>
DW_OP_plus // <42 or 87>
DW_OP_stack_value

Apart from the DW_OP_deref, this seems very good, I'm slightly worried about the assumption that the init value will always be the smaller one.
What happens if any of the two values is negative (because of the typeless DWARF <5 expression stack — not sure if this will be a problem).

The other expression which was suggested was :

// val * 42 | ((~val)&1) * 87
DW_OP_dup DW_OP_deref DW_OP_constu 42 DW_OP_mul DW_OP_swap
DW_OP_deref DW_OP_not DW_OP_lit1 DW_OP_and DW_OP_constu 87 DW_OP_mul
DW_OP_or DW_OP_stack_value

I have chosen to implement first because it is simpler.
I have also added simpler test also with concrete DW_AT_location description .

lib/Transforms/IPO/GlobalOpt.cpp

1644 ↗

(On Diff #110536)

typo: emit

[NikolaPrica]

val * (ValOther - ValInit) + ValInit:

If we are doing this expression, there should be an assertion in the code that ValInit < ValOther.

I have tested this for edge cases such as both negative values, LONG_MAX, LONG_INT etc and there was no problem for this values. Only flaw for this is ugly long DW_AT_location expression for long values.

 DW_OP_deref DW_OP_constu <ValMinus>
DW_OP_mul DW_OP_constu <ValInit> DW_OP_plus DW_OP_stack_value

Why does Val need to be dereferenced?

DW_OP_deref needs to be used because we have address of new variable on stack, not its value.

Apart from the DW_OP_deref, this seems very good, I'm slightly worried about the assumption that the init value will always be the smaller one.
What happens if any of the two values is negative (because of the typeless DWARF <5 expression stack — not sure if this will be a problem).

For dwarf versions lower than 4 gdb reads value of DW_AT_location expression as address giving message "Cannot access memory at address 0x1e". This may be a problem but it is still better than optimized-out?

[aprantl]

In D35994#854146, @NikolaPrica wrote:

val * (ValOther - ValInit) + ValInit:

If we are doing this expression, there should be an assertion in the code that ValInit < ValOther.

I have tested this for edge cases such as both negative values, LONG_MAX, LONG_INT etc and there was no problem for this values. Only flaw for this is ugly long DW_AT_location expression for long values.

Cool. Thanks for checking!

 DW_OP_deref DW_OP_constu <ValMinus>
DW_OP_mul DW_OP_constu <ValInit> DW_OP_plus DW_OP_stack_value

Why does Val need to be dereferenced?

DW_OP_deref needs to be used because we have address of new variable on stack, not its value.

Oh, because it is a global variable! Makes sense.

Apart from the DW_OP_deref, this seems very good, I'm slightly worried about the assumption that the init value will always be the smaller one.
What happens if any of the two values is negative (because of the typeless DWARF <5 expression stack — not sure if this will be a problem).

For dwarf versions lower than 4 gdb reads value of DW_AT_location expression as address giving message "Cannot access memory at address 0x1e". This may be a problem but it is still better than optimized-out?

That makes sense. I'm fine with not attempting to fix this.

test/Transforms/GlobalOpt/integer-bool-dwarf.ll

43 ↗

(On Diff #110684)

please strip out all non-essential attributes (everything in quotes)

[aprantl]

In fully-compliant mode we should just not emit any DWARF expression that ends with a DW_OP_stack_value in DWARF < 4. However, LLDB and GDB (at least the version Apple used to ship in Xcode) support a couple of expressions that are outside of the specification but do work in practice, so I'm reluctant to drop support for that by being stricter.
That said, Darwin has been using DWARF 4 for a while now, so an argument could be made that it is okay to regress on older deployment targets for the sake of correctness/standards compliance.

[NikolaPrica]

Updated test case

[dblaikie]

test/Transforms/GlobalOpt/integer-bool-dwarf.ll

83–91 ↗

(On Diff #113069)

Probably put these CHECK lines up the top, above the IR - that's where they usually go for test cases.

[NikolaPrica]

Updating test case.

llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

while (ExprCursor) {
     }
     case dwarf::DW_OP_plus_uconst:
       assert(LocationKind != Register);
       emitOp(dwarf::DW_OP_plus_uconst);
       emitUnsigned(Op->getArg(0));
       break;
     case dwarf::DW_OP_plus:
     case dwarf::DW_OP_minus:
+    case dwarf::DW_OP_mul:
       emitOp(Op->getOp());
       break;
     case dwarf::DW_OP_deref:
       assert(LocationKind != Register);
       if (LocationKind != Memory && isMemoryLocation(ExprCursor))
         // Turning this into a memory location description makes the deref
         // implicit.
         LocationKind = Memory;

llvm/trunk/lib/IR/DebugInfoMetadata.cpp

case dwarf::DW_OP_swap: {
       if (getNumElements() == 1)
         return false;
       break;
     }
     case dwarf::DW_OP_constu:
     case dwarf::DW_OP_plus_uconst:
     case dwarf::DW_OP_plus:
     case dwarf::DW_OP_minus:
+    case dwarf::DW_OP_mul:
     case dwarf::DW_OP_deref:
     case dwarf::DW_OP_xderef:
       break;
     }
   }
   return true;
 }
 

llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp

 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/IR/ValueHandle.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/IPO.h"
 #include "llvm/Transforms/Utils/CtorUtils.h"
 #include "llvm/Transforms/Utils/Evaluator.h"
GlobalVariable *NewGV = new GlobalVariable(Type::getInt1Ty(GV->getContext()),
                                              GV->getType()->getAddressSpace());
   NewGV->copyAttributesFrom(GV);
   GV->getParent()->getGlobalList().insert(GV->getIterator(), NewGV);
 
   Constant *InitVal = GV->getInitializer();
   assert(InitVal->getType() != Type::getInt1Ty(GV->getContext()) &&
          "No reason to shrink to bool!");
 
+  SmallVector<DIGlobalVariableExpression *, 1> GVs;
+  GV->getDebugInfo(GVs);
+
   // If initialized to zero and storing one into the global, we can use a cast
   // instead of a select to synthesize the desired value.
   bool IsOneZero = false;
-  if (ConstantInt *CI = dyn_cast<ConstantInt>(OtherVal))
+  if (ConstantInt *CI = dyn_cast<ConstantInt>(OtherVal)){
     IsOneZero = InitVal->isNullValue() && CI->isOne();
 
+    ConstantInt *CIInit = dyn_cast<ConstantInt>(GV->getInitializer());
+    uint64_t ValInit = CIInit->getZExtValue();
+    uint64_t ValOther = CI->getZExtValue();
+    uint64_t ValMinus = ValOther - ValInit;
+
+    for(auto *GVe : GVs){
+      DIGlobalVariable *DGV = GVe->getVariable();
+      DIExpression *E = GVe->getExpression();
+
+      // val * (ValOther - ValInit) + ValInit:
+      // DW_OP_deref DW_OP_constu <ValMinus>
+      // DW_OP_mul DW_OP_constu <ValInit> DW_OP_plus DW_OP_stack_value
+      E = DIExpression::get(NewGV->getContext(),
+                           {dwarf::DW_OP_deref,
+                            dwarf::DW_OP_constu,
+                            ValMinus,
+                            dwarf::DW_OP_mul,
+                            dwarf::DW_OP_constu,
+                            ValInit,
+                            dwarf::DW_OP_plus,
+                            dwarf::DW_OP_stack_value});
+      DIGlobalVariableExpression *DGVE =
+        DIGlobalVariableExpression::get(NewGV->getContext(), DGV, E);
+      NewGV->addDebugInfo(DGVE);
+    }
+  } else {
+    // FIXME: This will only emit address for debugger on which will
+    // be written only 0 or 1.
+    for(auto *GV : GVs)
+      NewGV->addDebugInfo(GV);
+  }
+
   while (!GV->use_empty()) {
     Instruction *UI = cast<Instruction>(GV->user_back());
     if (StoreInst *SI = dyn_cast<StoreInst>(UI)) {
       // Change the store into a boolean store.
       bool StoringOther = SI->getOperand(0) == OtherVal;
       // Only do this if we weren't storing a loaded value.
       Value *StoreVal;
       if (StoringOther || SI->getOperand(0) == InitVal) {