Issue with checkDLLAttributeRedeclaration in SemaDecl.cpp

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();
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… Do you think I should open a bug instead of discussing on the mailing list?

It appears the code was changed in the following revision:
Revision: 270686
Author: dzobnin
Message:
[ms][dll] #26935 Defining a dllimport function should cause it to be exported

If we have some function with dllimport attribute and then we have the function
definition in the same module but without dllimport attribute we should add
dllexport attribute to this function definition.
The same should be done for variables.

Example:
struct __declspec(dllimport) C3 {
~C3();
};
C3::~C3() {;} // we should export this definition.

Patch by Andrew V. Tischenko

Differential revision: http://reviews.llvm.org/D18953

Thank you for any help,

+Denis who wrote r270686

Yes, if you can report this on the Bugzilla that would be better.

If you can provide the preprocessed source and command-line (Clang
should be writing these to files in your temp dir when it crashes), I
can try to reduce it for you.

Thanks,
Hans

Hello again,

I’ve finally been able to reduce it to a working example. I can only get this when trying to compile incorrect code (a missing header), but I think it should not crash anyway:

#include “Zorg.h” // Non existing file

template<typename _Tp, std::size_t _Nm = 42>

struct array

{

};

class __declspec(dllimport) DBConnectivity

{

static bool myFunc(array args);

};

bool

DBConnectivity::myFunc(array args)

{

}

I’ll fill a Bugzilla issue as soon as I have an account (it looks like this is required, I’ve just sent a mail to llvm-admin@lists.llvm.org).

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.

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.

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