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

[David Blaikie]

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)

- David