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

[Devang Patel]

Dan,

On Wed, May 5, 2010 at 5:32 PM, Dan Gohman <gohman at apple.com> wrote:
>
> 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.

When S is null, it is a unknown location, so 1 is also not a valid
value here. I understand this was prompted by your beginScope change.
>
>>>
>>> -  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".

What exactly did you see as "unknown" ? some random value ? 0,0 ?

IMO, lack of location info is better then "unknown" location info. For
example, while single stepping instructions, "unknown" location
instructs the debugger to take you to "unknown" location which is not
at all helpful to end user. Where as lack of location info for a
instruction  lets the debugger to not take any action or assume
current last known location. Either way better then jumping to
"unknown" location in source file.  Plus, "unknown" location increases
line table size without any significant benefit.

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

You'd see extra labels in .s file.

I need a good reason to keep this change.
-
Devang