[cfe-commits] [PATCH] Improve handling of DLL import/export attributes

[Aaron Ballman]

On Tue, Sep 4, 2012 at 7:19 PM, João Matos <ripzonetriton at gmail.com> wrote:
> Thanks for the review. Comments below.
>
> On Tue, Sep 4, 2012 at 7:47 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> It would be good to specify *what* type it is accessible from.  Also,
>>
>> how would you "ignore" this warning if you wanted to (short of turning
>> it off entirely)?
>
>
> I can add a new warning group for all of the DLL-related warnings
> (-Wmicrosoft-dll?), would this address your concerns?

Yes and no.  I was thinking more along the lines of a way to turn off
the warning one-off.  Sort of like how you can silence "unused
variable" warnings using (void) or self-assignment.  It may not be
possible to do this here, so at the very least we would need a warning
group for it.  -Wmicrosoft-dll doesn't seem quite right to me, but I
don't have a better suggestion either.

>> Why is dllimport preferred over dllexport?  This is backwards from
>> what MSDN documents the behavior to be
>> (http://msdn.microsoft.com/en-us/library/twa2aw10).  Also, the second
>> if could be an else if to be more explicit that it can only be one or
>> the other.
>
>
> When the code reaches this point it should only have one attribute. Maybe I
> should add an assert / comment to make this clearer.

That would make sense, but I think that your patch doesn't address the
underlying problem.

void foo( void ) {
	__declspec( dllimport ) __declspec( dllexport ) int x = 5;
}

void foo( void ) {
	__declspec( dllexport ) __declspec( dllimport ) int x = 5;
}

These two examples produce different warnings.  The latter says that
dllimport is ignored but then fails to ignore it, the former fails to
tell you it's ignoring the dllimport.

>>
>>  When will this accessibility check be implemented?  Since this is all
>> new code, I'd rather not add the FIXME unless this is a major
>> undertaking worthy of a separate patch.
>
>
> I'm not sure about this. It seems to me a lot more complex than it first
> appears. What if a friend function or class that is exported accesses some
> of the private class members? Then you also need to export it, and it seems
> pretty hard to this check in a fast way. For now I just export everything. I
> commented about this on the bug tracker where I first posted the patch and
> asked for some opinions about this, but no one gave any suggestions, so I
> suggest we try to optimize this later.

Okay, that's fair.

>> So I think the diagnostic should point to the current record's
>> dllexport and then have a note pointing to the base class declaration
>> tying the two together.  Also, I wonder if we could add a fixit to
>> supply the dllexport for the definition of the base class?
>
>
> Well a note is fine, but I think pointing to the base class points to the
> real problem. Adding a note is fine but will be more verbose and the real
> problem will be in the note and not the main diagnostic.

The base class's declaration is mismatched with the declspec on the
subclass, which is why I think it would make the most sense to point
to the declspec and then note to the base class declaration.  This
tells the user "these two things relate."

> I thought about the fix it, but this sometimes happens for system types like
> the STL and MS's stance about it has been to just ignore it, I was worried
> that the fix it system might try to fix the system headers.

We already have ways to ignore system header issues.

>> I wonder if this is a case for a fixit, since we could conceivably
>> just add extern to the declaration and continue.
>
> Sure I thought of adding a fixit, but thought it could lead to it doing the
> wrong thing in some cases where the attribute is badly applied. I don't
> think it's worth it to add a fix it here.

That is true -- there are two ways to fix this, one is to apply the
extern the other is to remove the declspec.  So nevermind, fixit
likely won't help here.

>>
>> If this was a variable declaration, didn't you just set the linkage by
>> setting the storage class on VD?
>
>
> Yeah, but this should catch the problem on other declaration kinds.

Yes, it will.  But it's still odd (and inefficient) to set the storage
class then turn around and ask for it again.

>> > -  // Attribute can be applied only to functions or variables.
>> > -  FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
>> > -  if (!FD && !isa<VarDecl>(D)) {
>> > -    // Apparently Visual C++ thinks it is okay to not emit a warning
>> > -    // in this case, so only emit a warning when -fms-extensions is not
>> > -    // specified.
>>
>> That's because you can dllimport a class (at least you can according
>> to http://msdn.microsoft.com/en-us/library/81h27t8c).  I don't think
>> this is a valid diagnostic.
>
>
> Indeed. But I am confused here, are you talking about the code that was
> removed?

Sorry for the confusion, yes, I was talking about code that was removed.  Oops!

>> Also, you are missing a check to ensure dllimport hasn't already been
>> specified.  Currently you can do __declspec( dllimport ) __declspec(
>> dllimport ) int x;  and it will not warn.
>
> Shouldn't that be checked by a more general duplicated attribute system? If
> this is not true, I can add a new diagnostic.

It should be somewhere -- I think it would make more sense to check
for duplicates in a generic way, yes.

>> Why are you getting rid of this entire file?  If you have C++-specific
>> tests to add for things like classes, then you should add a C++ file
>> and put those tests there instead of getting rid of the entire C file.
>>  Also, it might be worth testing export/import structures from C.
>
>
> That was actually how I had it, but I remember reading someone asking to
> keep the number of test cases down (I think it was Chandler, and this was
> not to me specifically) so I merged it into one.

Generally speaking, it's better to add test cases to an existing file
than it is to add an entirely new file.  However, your approach here
changes the nature of some of the tests which is a bad thing.  We need
those tests to continue to work in C, so moving them to a C++ source
file reduces the test coverage.  I would create a C++ file that has
the C++-specific tests in it, but continue to add to the C test file
cases which make sense.

~Aaron