[PATCH] Ensure __va_list_tag has default visibility

[Richard Smith]

LGTM, thanks!

On Tue, Jan 6, 2015 at 12:03 PM, Stephan Bergmann <sbergman at redhat.com>
wrote:

> On 12/19/2014 05:13 AM, Richard Smith wrote:
>
>> On Tue, Dec 16, 2014 at 1:47 AM, Stephan Bergmann <sbergman at redhat.com
>> <mailto:sbergman at redhat.com>> wrote:
>>
>>     Is there any update on this?
>>
>>
>> Please move the attribute to buildImplicitRecord; this attribute should
>> be applied to all implicitly-declared record types. Also, please
>> reformat the patch to 80 columns and add a testcase.
>>
>
> Done, see attached va_list_tag_default_visibility.patch.  (I added the
> test to test/CodeGenCXX/ so that it can #include <typeinfo>.)
>
>      On 07/09/2014 10:37 AM, Alp Toker wrote:
>>
>>         On 09/07/2014 11:29, Alp Toker wrote:
>>
>>             On 09/07/2014 09:55, Stephan Bergmann wrote:
>>
>>                 ping
>>
>>                 On 06/25/2014 05:45 PM, Stephan Bergmann wrote:
>>
>>                     I stumbled across this on Linux with
>>                     -fvisibility=hidden when
>>                     -fsanitize=function reported a false positive for an
>>                     indirect call to a
>>                     function with a va_list (aka __builtin_va_list, aka
>>                     __va_list_tag
>>                     (*)[1]) parameter.  Because __va_list_tag was
>>                     considered hidden, the
>>                     RTTI for the function's type was not exported from
>>                     the two shared
>>                     libraries involved, so the equality check failed.
>>
>>                     This would apparently also need to be fixed in the
>> other
>>                     Create*BuiltinVaListDecl variants, but I'm not sure
>>                     (a) whether the
>>                     added const_cast is really deemed appropriate here
>>                     (though I guess so,
>>                     given the large number of existing similar
>>                     const_casts across
>>                     ASTContext.cpp), and (b) what the preferred way
>>                     would be to break that
>>                     long line. ;)
>>
>>
>>             Hi Stephan,
>>
>>             I wrote the buildImplicitRecord() utility, and in principle
>> the
>>             attribute could be added centrally to that function -- if
>>             these are
>>             the semantics we want for most built-in types, as you
>>             suggest, it
>>             makes sense to centralize the decision.
>>
>>             Another option would be to calculate the special linkage in
>>             computeLVForDecl() instead of using an attribute, but it
>> doesn't
>>             really matter either way I think as the result is the same.
>>
>>             The bigger question is whether (1) default visibility is
>>             correct for
>>             all built-in types currently being generated by
>>             buildImplicitRecord()
>>             and (2) whether this is a valid choice for all targets we
>>             support
>>             including MSVC drop-in compatibility mode.
>>
>>             To help answer question (1), the affected built-in types
>>             would be:
>>
>>             lib/AST/ASTContext.cpp:    Float128StubDecl =
>>             buildImplicitRecord("____float128");
>>             lib/AST/ASTContext.cpp:    CFConstantStringTypeDecl =
>>             buildImplicitRecord("__NSConstantString");
>>             lib/AST/ASTContext.cpp:    RecordDecl *ObjCSuperTypeDecl =
>>             buildImplicitRecord("objc___super");
>>             lib/AST/ASTContext.cpp:  RD =
>>             buildImplicitRecord("__block___descriptor");
>>             lib/AST/ASTContext.cpp:  RD =
>>             buildImplicitRecord("__block___descriptor_withcopydispose");
>>             lib/AST/ASTContext.cpp:  RecordDecl *VaListTagDecl =
>>             Context->buildImplicitRecord("____va_list");
>>             lib/AST/ASTContext.cpp:  VaListTagDecl =
>>             Context->buildImplicitRecord("____va_list_tag");
>>             lib/AST/ASTContext.cpp:  VaListTagDecl =
>>             Context->buildImplicitRecord("____va_list_tag");
>>             lib/AST/ASTContext.cpp:  RecordDecl *VaListDecl =
>>             Context->buildImplicitRecord("____va_list");
>>             lib/AST/ASTContext.cpp:  VaListTagDecl =
>>             Context->buildImplicitRecord("____va_list_tag");
>>             lib/CodeGen/CodeGenModule.cpp:    RecordDecl *D =
>>             Context.buildImplicitRecord("____builtin_NSString");
>>             lib/CodeGen/CodeGenModule.cpp:    RecordDecl *D =
>>             Context.buildImplicitRecord("____objcFastEnumerationState");
>>             lib/Sema/Sema.cpp:
>>             PushOnScopeChains(Context.__buildImplicitRecord("type___
>> info",
>>             TTK_Class),
>>
>>
>>         There are also the implicit typedefs -- I don't know what the
>>         visibility/export deal is for those:
>>
>>
>>         lib/AST/ASTContext.cpp:    Int128Decl =
>>         buildImplicitTypedef(Int128Ty,
>>         "__int128_t");
>>         lib/AST/ASTContext.cpp:    UInt128Decl =
>>         buildImplicitTypedef(__UnsignedInt128Ty, "__uint128_t");
>>         lib/AST/ASTContext.cpp:
>>         buildImplicitTypedef(__getObjCIdType(),
>>         "instancetype");
>>         lib/AST/ASTContext.cpp:    ObjCIdDecl = buildImplicitTypedef(T,
>>         "id");
>>         lib/AST/ASTContext.cpp:    ObjCSelDecl = buildImplicitTypedef(T,
>>         "SEL");
>>         lib/AST/ASTContext.cpp:    ObjCClassDecl = buildImplicitTypedef(T,
>>         "Class");
>>         lib/AST/ASTContext.cpp:  return Context->buildImplicitTypedef(
>> __T,
>>         "__builtin_va_list");
>>         lib/AST/ASTContext.cpp:  return Context->buildImplicitTypedef(
>> __T,
>>         "__builtin_va_list");
>>         lib/AST/ASTContext.cpp:  return
>>         Context->buildImplicitTypedef(__VaListTagType,
>> "__builtin_va_list");
>>         lib/AST/ASTContext.cpp:
>>         Context->buildImplicitTypedef(__VaListTagType,
>>         "__va_list_tag");
>>         lib/AST/ASTContext.cpp:  return
>>         Context->buildImplicitTypedef(__VaListTagArrayType,
>>         "__builtin_va_list");
>>         lib/AST/ASTContext.cpp:
>>         Context->buildImplicitTypedef(__VaListTagType,
>>         "__va_list_tag");
>>         lib/AST/ASTContext.cpp:  return
>>         Context->buildImplicitTypedef(__VaListTagArrayType,
>>         "__builtin_va_list");
>>         lib/AST/ASTContext.cpp:  return
>>         Context->buildImplicitTypedef(__IntArrayType,
>> "__builtin_va_list");
>>         lib/AST/ASTContext.cpp:  return Context->buildImplicitTypedef(
>> __T,
>>         "__builtin_va_list");
>>         lib/AST/ASTContext.cpp:
>>         Context->buildImplicitTypedef(__VaListTagType,
>>         "__va_list_tag");
>>         lib/AST/ASTContext.cpp:  return
>>         Context->buildImplicitTypedef(__VaListTagArrayType,
>>         "__builtin_va_list");
>>         lib/Sema/Sema.cpp:
>>         PushOnScopeChains(Context.__buildImplicitTypedef(T,
>>         Name), TUScope);
>>
>>         Alp.
>>
>>
>>             I'll need help answering those questions from
>>             linkage/visibility folks
>>             on the list before going forward with a change like this.
>>
>>             Thanks
>>             Alp.
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150114/924bd4e8/attachment.html>