[PATCH] Anonymous namespaces are missing import DW_TAG_imported_module.
Duncan P. N. Exon Smith
dexonsmith at apple.com
Wed Feb 25 21:12:19 PST 2015
> On 2015 Feb 25, at 19:43, Romanova, Katya <Katya_Romanova at playstation.sony.com> wrote:
>
> Hi,
> I'm having technical problems with Phabricator. Per Duncun request, I attached a patch for the review http://reviews.llvm.org/D7895
> Thanks!
> Katya.
I'm not the right person to review the high-level, but I have a
minor comment below.
> Index: lib/CodeGen/AsmPrinter/DwarfUnit.cpp
> ===================================================================
> --- lib/CodeGen/AsmPrinter/DwarfUnit.cpp (revision 230313)
> +++ lib/CodeGen/AsmPrinter/DwarfUnit.cpp (working copy)
> @@ -1207,12 +1207,20 @@
> DIE &NDie = createAndAddDIE(dwarf::DW_TAG_namespace, *ContextDIE, NS);
>
> StringRef Name = NS.getName();
> - if (!Name.empty())
> + bool Anonymous = Name.empty();
Seems a little funny to me to have a variable for this. `Name.empty()`
should be cheap enough, and the string in the `else` case already
documents the meaning.
However, if you do go with a variable, something like `IsAnonymous`
matches the coding standards better.
Maybe a better option would be to add a `DINamespace::isAnonymous()`
accessor that does the check, and just use it here. (Switching to the
accessor should be a separate commit though.)
> + if (!Anonymous)
> addString(NDie, dwarf::DW_AT_name, NS.getName());
> else
> Name = "(anonymous namespace)";
> DD->addAccelNamespace(Name, NDie);
> addGlobalName(Name, NDie, NS.getContext());
> + if (Anonymous) {
> + // For anonymous namespaces, add a DW_TAG_imported_module tag
> + // containing an DW_AT_import attribute with the reference to this
> + // namespace entry.
> + DIE &IDie = createAndAddDIE(dwarf::DW_TAG_imported_module, *ContextDIE);
> + addDIEEntry(IDie, dwarf::DW_AT_import, NDie);
> + }
> addSourceLine(NDie, NS);
> return &NDie;
> }
>
>
> -----Original Message-----
> From: Duncan P. N. Exon Smith [mailto:dexonsmith at apple.com]
> Sent: Wednesday, February 25, 2015 7:35 PM
> To: reviews+D7895+public+7827be49c0b04087 at reviews.llvm.org
> Cc: Romanova, Katya; Robinson, Paul; David Blaikie; Eric Christopher; llvm-commits at cs.uiuc.edu
> Subject: Re: [PATCH] Anonymous namespaces are missing import DW_TAG_imported_module.
>
>
>> On 2015 Feb 25, at 19:19, Katya Romanova <Katya_Romanova at playstation.sony.com> wrote:
>>
>> I have already created a review for this bug a few hours ago, but it didn't reach the full audience. I didn't add llvm--dev at first when I created the review. Apparently, adding it later doesn't have any effect and the review wasn't sent out to the mailing list. I abandoned the old review and created this one.
>
> Sadly, there's still no patch on the list. I'm not sure what you need to do to make phabricator post it for you.
>
> Feel free to go "low-tech" and just attach the patch to an email (maybe in reply to this one?).
>
> (BTW, Phabricator isn't required for reviews; it's just a tool that some find useful (IIUC because downloading/applying/editing patches is difficult in some environments?).)
>
>> Here are a couple of comments that were added to the old review by the people who were specified as the reviewers:
>>
>> from David Blaikie:
>>
>>>> I'm not sure if this is really a bug in the debug info - seems like it might be reasonably treated as a bug in the debugger?
>>
>>>
>>
>>
>> from Paul Robinson:
>>
>>>> Our debugger guys get pretty pedantic about interpreting the DWARF spec. I know GDB will auto-import info from an anonymous namespace, but:
>>
>>>
>>
>>>>
>>
>>>
>>
>>>> the size cost here is really minimal (what, 5 or 6 bytes per
>>>> anonymous namespace scope);
>>
>>>
>>
>>>> they claimed that deciding whether to skip the namespace based on
>>>> the name would be a problem for them;
>>
>>>
>>
>>>> the compiler implementation is obviously trivial;
>>
>>>
>>
>>
>>
>>
>>>> so I didn't really feel motivated to argue the point.
>>
>>>
>>
>>
>>
>> http://reviews.llvm.org/D7895
>>
>> EMAIL PREFERENCES
>> http://reviews.llvm.org/settings/panel/emailpreferences/
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
> <anon2.txt>
More information about the llvm-commits
mailing list