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

[Eric Christopher]

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.

-eric