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

[Hans Wennborg via cfe-dev]

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.