[llvm] r205286 - DebugInfo: Emit relocation to debug_line section when emitting asm for asm

[Eric Christopher]

On Thu, Apr 3, 2014 at 8:29 AM, David Blaikie <dblaikie at gmail.com> wrote:
> On Thu, Apr 3, 2014 at 4:19 AM, Eric Christopher <echristo at gmail.com> wrote:
>> On Tue, Apr 1, 2014 at 7:06 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>> On Tue, Apr 1, 2014 at 3:38 PM, Eric Christopher <echristo at gmail.com> wrote:
>>>> On Tue, Apr 1, 2014 at 12:35 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>> Author: dblaikie
>>>>> Date: Tue Apr  1 02:35:52 2014
>>>>> New Revision: 205286
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=205286&view=rev
>>>>> Log:
>>>>> DebugInfo: Emit relocation to debug_line section when emitting asm for asm
>>>>>
>>>>> I don't think this is reachable by any frontend (why would you transform
>>>>> asm to asm+debug info?) but it helps tidy up some of this code, avoid
>>>>> the weird special case of "emit the first CU, store the label, then emit
>>>>> the rest" in MCDwarfLineTable::Emit by instead having the
>>>>> DWARF-for-assembly case use the same codepath as DwarfDebug.cpp, by
>>>>> registering the label of the debug_line section, thus causing it to be
>>>>> emitted. (with a special case in asm output to just emit the label since
>>>>> asm output uses the .loc directives, etc, rather than the debug_loc
>>>>> directly)
>>>>>
>>>>
>>>> Looks very nice, thanks.
>>>>
>>>>>    // Check to see there are no empty DwarfFile slots.
>>>>> -  const SmallVectorImpl<MCDwarfFile> &MCDwarfFiles =
>>>>> -      getContext().getMCDwarfFiles();
>>>>> -  for (unsigned i = 1; i < MCDwarfFiles.size(); i++) {
>>>>> -    if (MCDwarfFiles[i].Name.empty())
>>>>> -      TokError("unassigned file number: " + Twine(i) + " for .file directives");
>>>>> +  const auto &LineTables = getContext().getMCDwarfLineTables();
>>>>> +  if (!LineTables.empty()) {
>>>>> +    unsigned Index = 0;
>>>>> +    for (const auto &File : LineTables.begin()->second.getMCDwarfFiles()) {
>>>>> +      if (File.Name.empty() && Index != 0)
>>>>> +        TokError("unassigned file number: " + Twine(Index) +
>>>>> +                 " for .file directives");
>>>>> +      ++Index;
>>>>> +    }
>>>>>    }
>>>>
>>>> Might be silly, but update the comment to reflect that we're also
>>>> looking for invalid ones? Not that you changed anything here, but ...
>>>
>>> I'm not sure quite what distinction you're drawing here between empty
>>> and invalid? Is it just that you prefer to describe these as "invalid"
>>> rather than empty?
>>>
>>> The logic here is that if an asm file has 2 .file directives, one for
>>> file 1 and another for file 3, then file 2 is "unassigned" (since they
>>> must be contiguous). Alternatively we could say that file 3 (and all
>>> subsequent files?) is "invalid".
>>>
>>> Or just update the comment to use the same terminology as the error -
>>> "unassigned"?
>>
>> Bikeshed color: Orange.
>>
>> I like invalid here since they have to be contiguous.
>
> Which file number would you like to be reported as invalid, then? If
> we have {1, 3, 4} we currently report 2 as missing. If we want to
> report invalid numbers - is 3 invalid, or both 3 and 4? It seems weird
> to describe 2 as invalid when it's not even present. (describing it as
> missing or unassigned seems OK)
>

Hrm. Good question since the files aren't necessarily supposed to be
contiguous in the listing.

Bah, nevermind.

-eric