[cfe-dev] Issue with checkDLLAttributeRedeclaration in SemaDecl.cpp

[Hans Wennborg via cfe-dev]

Thanks! I'll try to look into the bug properly this week.

On Sat, Nov 19, 2016 at 4:48 AM, Loïc Joly <l.joly at castsoftware.com> wrote:
> I've been able to add a Bugzilla issue: https://llvm.org/bugs/show_bug.cgi?id=31069
>
> I think you're right in the first part of your comment, when I ran the code under the debugger, I could see that some attribute merging happens in the normal case, not in the broken case.
>
> I'm not knowledgeable enough about Clang to have an opinion about the second part.
>
> ---
> Loïc Joly
>
> -----Message d'origine-----
> De : hwennborg at google.com [mailto:hwennborg at google.com] De la part de Hans Wennborg
> Envoyé : mercredi 16 novembre 2016 01:58
> À : Loïc Joly <l.joly at castsoftware.com>
> Cc : cfe-dev at lists.llvm.org
> Objet : Re: [cfe-dev] Issue with checkDLLAttributeRedeclaration in SemaDecl.cpp
>
> On Mon, Nov 14, 2016 at 9:39 AM, Loïc Joly via cfe-dev <cfe-dev at lists.llvm.org> wrote:
>> Hello,
>>
>> When running Clang 3.9.0 on some Windows code (I've not yet been able
>> to reproduce the issue with a small example), we get a crash. I've
>> been able to see that the code crashes in
>> checkDLLAttributeRedeclaration, more specifically in the following code:
>>
>>   if (OldImportAttr && !HasNewAttr && !IsInline && !IsStaticDataMember &&
>>       !NewDecl->isLocalExternDecl() && !IsQualifiedFriend) {
>>     if (IsMicrosoft && IsDefinition) {
>>       S.Diag(NewDecl->getLocation(),
>>              diag::warn_redeclaration_without_import_attribute)
>>           << NewDecl;
>>       S.Diag(OldDecl->getLocation(), diag::note_previous_declaration);
>>       NewDecl->dropAttr<DLLImportAttr>();
>>       NewDecl->addAttr(::new (S.Context) DLLExportAttr(
>>           NewImportAttr->getRange(), S.Context,
>>           NewImportAttr->getSpellingListIndex()));
>>     } else {
>>
>> We try to read some data from NewImportAttr, but in our case,
>> NewImportAttr is null (which is compatible with the !HasNewAttr test).
>> I don't really understand the logic of this code, this is why I ask
>> for advice before changing anything...
>
> I think the code assumes that even though HasNewAttr is false, the new declaration would have inherited a dllimport attribute from the old declaration.
>
> That's what usually happens, but not in the case you're running into.
> It's interesting that this only seems to reproduce during error recovery. Maybe that makes the attribute not get inherited?
>
> Anyway, I wonder if the right fix is to not try to get the SourceRange from NewImportAttr. It seems a bit odd since the DLLExport attribute is never actually spelled out in the source. I'm not sure what we usually do for implicit attributes like this.
>
> And speaking of implicit, the attribute should be marked as implicit while we're here.