[llvm-commits] [llvm] r103135 - /llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

[Dan Gohman]

On May 5, 2010, at 5:12 PM, Devang Patel wrote:

> Dan,
> 
> This does not make sense. See below...
> 
> On May 5, 2010, at 4:41 PM, Dan Gohman wrote:
> 
>> Author: djg
>> Date: Wed May  5 18:41:32 2010
>> New Revision: 103135
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=103135&view=rev
>> Log:
>> Emit debug info for MachineInstrs with unknown debug locations, instead
>> of just letting them inherit the debug locations of adjacent instructions.
>> 
>> Debug info should aim to be either accurate or absent.
>> 
>> Modified:
>>    llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>> 
>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=103135&r1=103134&r2=103135&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Wed May  5 18:41:32 2010
>> @@ -2150,8 +2150,20 @@
>> void DwarfDebug::beginScope(const MachineInstr *MI) {
>>   // Check location.
>>   DebugLoc DL = MI->getDebugLoc();
>> -  if (DL.isUnknown())
>> +  if (DL.isUnknown()) {
>> +    // This instruction has no debug location. If the preceding instruction
>> +    // did, emit debug location information to indicate that the debug
>> +    // location is now unknown.
>> +    MCSymbol *Label = NULL;
>> +    if (DL == PrevInstLoc)
>> +      Label = PrevLabel;
> 
> Here, Label is not used afterwords.

Oops. This is fixed now.

> 
>> +    else {
>> +      Label = recordSourceLine(DL.getLine(), DL.getCol(), 0);
> 
> If DL is unknown then DL.getLine(), DL.getCol() are not useful to record source location.

That's intentional. I could change this to recordSourceLine(0, 0, 0)
if you think that's more clear.

>> +      PrevInstLoc = DL;
>> +      PrevLabel = Label;
> 
>  And here PrevInstLoc and PrevLable were updated when a label for valid location was emitted. Instead you're overriding valid PrevInstLoc with Unknown debug location.

That's also intentional. PrevInstLoc will then stay unknown
until an instruction with a known location is visited.


> 
>> +    }
>>     return;
>> +  }
> 
> 
>>   MDNode *Scope = DL.getScope(Asm->MF->getFunction()->getContext());
>> 
>> @@ -2564,23 +2576,28 @@
>>   StringRef Dir;
>>   StringRef Fn;
>> 
>> -  DIDescriptor Scope(S);
>> -  if (Scope.isCompileUnit()) {
>> -    DICompileUnit CU(S);
>> -    Dir = CU.getDirectory();
>> -    Fn = CU.getFilename();
>> -  } else if (Scope.isSubprogram()) {
>> -    DISubprogram SP(S);
>> -    Dir = SP.getDirectory();
>> -    Fn = SP.getFilename();
>> -  } else if (Scope.isLexicalBlock()) {
>> -    DILexicalBlock DB(S);
>> -    Dir = DB.getDirectory();
>> -    Fn = DB.getFilename();
>> -  } else
>> -    assert(0 && "Unexpected scope info");
>> +  unsigned Src = 1;
>> +  if (S) {
>> +    DIDescriptor Scope(S);
>> +
>> +    if (Scope.isCompileUnit()) {
>> +      DICompileUnit CU(S);
>> +      Dir = CU.getDirectory();
>> +      Fn = CU.getFilename();
>> +    } else if (Scope.isSubprogram()) {
>> +      DISubprogram SP(S);
>> +      Dir = SP.getDirectory();
>> +      Fn = SP.getFilename();
>> +    } else if (Scope.isLexicalBlock()) {
>> +      DILexicalBlock DB(S);
>> +      Dir = DB.getDirectory();
>> +      Fn = DB.getFilename();
>> +    } else
>> +      assert(0 && "Unexpected scope info");
>> +
>> +    Src = GetOrCreateSourceID(Dir, Fn);
>> +  }
> 
> I am not sure what exactly this part achieves other then increasing indentation.

When S is null, Src should be set to 1.

>> 
>> -  unsigned Src = GetOrCreateSourceID(Dir, Fn);
>>   MCSymbol *Label = MMI->getContext().CreateTempSymbol();
>>   Lines.push_back(SrcLineInfo(Line, Col, Src, Label));
>> 
>> @@ -2967,8 +2984,6 @@
>>       MCSymbol *Label = LineInfo.getLabel();
>>       if (!Label->isDefined()) continue; // Not emitted, in dead code.
>> 
>> -      if (LineInfo.getLine() == 0) continue;
>> -
>>       if (Asm->isVerbose()) {
>>         std::pair<unsigned, unsigned> SrcID =
>>           getSourceDirectoryAndFileIds(LineInfo.getSourceID());
> 
> Can you send me small test case where this patch improves debug info ?

I took a simple program and compiled it with -g -emit-llvm. I edited
the output and deleted the !dbg parts from some of the instructions in
the middle. I then ran CodeGen and produced an object file, and then
checked that the debug info for those instructions was in fact set
to "unknown".

I don't know how to write a test for this.

Dan