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

[João Matos]

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?


> If you're going to hoist things from the if, why not go the whole way?
>

I didn't want to change much of the existing code but I can do that.


> You should probably also be checking the storage class specifier since
> dllimport must have external linkage.  Eg)
>
> static __declspec( dllimport ) int l;  // This is an error
>

This check should be done in HandleDLLShared(). I'll add one more test case
for this.


> 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.


> Might want a "previously declared here" note to point out that this
> conflict is happening because of the class.
>

Alright.


>  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.
                                                         ^

> 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.

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.


> > +  // "As a rule, everything that is accessible to the DLL's client
> (according
> > +  // to C++ access rules) should be part of the exportable interface.
> > +  // This includes private data members referenced in inline functions."
> > +  // FIXME: Add more checks for those complex cases.
>
> Why not add those checks right now?  Or at the very least, make the
> FIXME something actionable.  What complex cases still need covering?
>

Well I already covered some of this in the comment above. It seems to me
that friend functions make this harder than it seems.


> 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.


> 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.


> > -  // 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?

I don't think this is valid either as it is explicitly allowed by MSVC
> (even though I think it's a bit strange):
>

Yes, I know this is supported, but I'm not trying to support yet with this
patch. Selective members and inline / friends referencing private data will
come later.


> 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.
 > -  // Attribute can be applied only to functions or variables.

> > -  FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
> > -  if (!FD && !isa<VarDecl>(D)) {
>
> Again, I don't think this is correct since you can dllexport a class.
>

Again, are you commenting the removed code?

> -    S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
> > -      << Attr.getName() << 2 /*variable and function*/;
> > -    return;
> > -  }
> > -
> >    // Currently, the dllexport attribute is ignored for inlined
> functions, unless
> >    // the -fkeep-inline-functions flag has been used. Warning is emitted;
> > +  // FIXME: MSDN says this should work for inline functions in all
> cases.
>
> Are you intending to address this FIXME?
>

Not at the moment, there is much more important stuff to fix in the MS
support. But eventually I'd like to tackle these weirder cases.


> > +  FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
> >    if (FD && FD->isInlineSpecified()) {
> >      // FIXME: ... unless the -fkeep-inline-functions flag has been used.
>
> Or this one?
>

This was already here and it should be fixed when all the inline support is
reworked.

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.


> I'd like to see more test cases as well, such as ones missing from
> MSDN, as well as edge cases like duplicate dllimport or dllexport
> attributes, etc
>

Sure, these can be added.

-- 
João Matos
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120905/6d91e794/attachment.html>