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

Wed Sep 5 09:55:21 PDT 2012

On Wed, Sep 5, 2012 at 1:19 AM, 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?
>

For most warnings there is a note (or a few notes) explaining the changes
to make into the code so that the warning no longer fires.

For example, if you write    if (x = 4)   you get a warning and two notes,
the notes suggest to either change = into == OR to add parentheses around x
= 4.

-- Matthieu

>
>
>> 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
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120905/590a76ed/attachment.html>