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

[Loïc Joly via cfe-dev]

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.