Skip to content

[Clang] Allow simpler visibility annotations when targeting win32 and mingw #133699

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[Clang] Allow simpler visibility annotations when targeting win32 and mingw #133699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions clang/include/clang/Basic/Attr.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4185,6 +4185,14 @@ def DLLExport : InheritableAttr, TargetSpecificAttr<TargetHasDLLImportExport> {
let Documentation = [DLLExportDocs];
}

def DLLExportOnDecl : InheritableAttr, TargetSpecificAttr<TargetHasDLLImportExport> {
Copy link
Collaborator

@erichkeane erichkeane Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I'm not a huge fan of this TBH. An attribute for the sole purpose of diagnostics is a little novel and not particularly in keeping with our "represent the AST" nature of attributes. Is there really no way we can figure this out later?

Copy link
Contributor Author

@philnik777 philnik777 May 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't represent invalid attribute in the AST currently I don't think there is.

// This attribute is only used to warn if there was a `__declspec(dllexport)`
// on a declaration, but not on the defintion of an explciit instantiation
Copy link
Member

@mstorsjo mstorsjo Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: explciit

Copy link
Collaborator

@erichkeane erichkeane Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still this typo.

let Spellings = [];
let Subjects = SubjectList<[CXXRecord]>;
let Documentation = [InternalOnly];
}

def DLLExportStaticLocal : InheritableAttr, TargetSpecificAttr<TargetHasDLLImportExport> {
// This attribute is used internally only when -fno-dllexport-inlines is
// passed. This attribute is added to inline functions of a class having the
Expand Down
6 changes: 5 additions & 1 deletion clang/include/clang/Basic/DiagnosticGroups.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,9 @@ def Dangling : DiagGroup<"dangling", [DanglingAssignment,
ReturnStackAddress]>;
def DistributedObjectModifiers : DiagGroup<"distributed-object-modifiers">;
def DllexportExplicitInstantiationDecl : DiagGroup<"dllexport-explicit-instantiation-decl">;
def DllexportExplicitInstantiation :
DiagGroup<"dllexport-explicit-instantiation",
[DllexportExplicitInstantiationDecl]>;
def ExcessInitializers : DiagGroup<"excess-initializers">;
def ExpansionToDefined : DiagGroup<"expansion-to-defined">;
def FlagEnum : DiagGroup<"flag-enum">;
Expand Down Expand Up @@ -870,7 +873,8 @@ def NSReturnsMismatch : DiagGroup<"nsreturns-mismatch">;

def IndependentClassAttribute : DiagGroup<"IndependentClass-attribute">;
def UnknownAttributes : DiagGroup<"unknown-attributes">;
def IgnoredAttributes : DiagGroup<"ignored-attributes">;
def IgnoredAttributes : DiagGroup<"ignored-attributes",
[DllexportExplicitInstantiation]>;
Comment on lines +876 to +877
Copy link
Contributor

@cor3ntin cor3ntin Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this change? It does not seem correct to me given the attribute has no spelling

Copy link
Contributor Author

@philnik777 philnik777 Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need some way to track that the attribute has been applied, even though it's being ignored. That's what the new attribute is for. This diagnostic is issued if the attribute has been ignored on the declaration and there is no attribute on the definition. Morally I'm still warning about the __declspec(dllexport), even if it's internally a different attribute.

def Attributes : DiagGroup<"attributes", [UnknownAttributes,
IgnoredAttributes]>;
def UnknownSanitizers : DiagGroup<"unknown-sanitizers">;
Expand Down
12 changes: 11 additions & 1 deletion clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3736,9 +3736,19 @@ def warn_attribute_dllimport_static_field_definition : Warning<
def warn_attribute_dllexport_explicit_instantiation_decl : Warning<
"explicit instantiation declaration should not be 'dllexport'">,
InGroup<DllexportExplicitInstantiationDecl>;
def warn_attribute_dllexport_explicit_instantiation_def : Warning<
def warn_attr_dllexport_explicit_inst_def : Warning<
"'dllexport' attribute ignored on explicit instantiation definition">,
InGroup<DllexportExplicitInstantiation>;
def warn_attr_dllexport_explicit_inst_def_mismatch : Warning<
"'dllexport' attribute ignored on explicit instantiation definition">,
InGroup<IgnoredAttributes>;
def note_prev_decl_missing_dllexport : Note<
"'dllexport' attribute is missing on previous declaration">;
def warn_dllexport_on_decl_ignored : Warning<
"explicit instantiation definition is not exported without 'dllexport'">,
InGroup<IgnoredAttributes>;
def note_dllexport_on_decl : Note<
"'dllexport' attribute on the declaration is ignored">;
def warn_attribute_exclude_from_explicit_instantiation_local_class : Warning<
"%0 attribute ignored on local class%select{| member}1">,
InGroup<IgnoredAttributes>;
Expand Down
5 changes: 4 additions & 1 deletion clang/lib/Sema/SemaDeclCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6606,7 +6606,10 @@ void Sema::checkClassLevelDLLAttribute(CXXRecordDecl *Class) {
if (ClassExported && !ClassAttr->isInherited() &&
TSK == TSK_ExplicitInstantiationDeclaration &&
!Context.getTargetInfo().getTriple().isWindowsGNUEnvironment()) {
Class->dropAttr<DLLExportAttr>();
if (auto *DEA = Class->getAttr<DLLExportAttr>()) {
Class->addAttr(DLLExportOnDeclAttr::Create(Context, DEA->getLoc()));
Copy link
Collaborator

@erichkeane erichkeane Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly would like this better if we just kept the DLLExportAttr and added a bool flag to it for ignoredBecauseItsOnaDeclWhereItWouldntWorkForVariousReasons in the extra members section.

Copy link
Contributor Author

@philnik777 philnik777 May 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that would be possible. The main problem I see with this is that we'd have to check in a bunch of places whether the attribute is actually valid, and not just whether it exists (AFAICT it's usually assumed that if an attribute is in the AST it is valid).

Class->dropAttr<DLLExportAttr>();
}
return;
}

Expand Down
20 changes: 18 additions & 2 deletions clang/lib/Sema/SemaTemplate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9845,13 +9845,29 @@ DeclResult Sema::ActOnExplicitInstantiation(
// mode, if a previous declaration of the instantiation was seen.
for (const ParsedAttr &AL : Attr) {
if (AL.getKind() == ParsedAttr::AT_DLLExport) {
Diag(AL.getLoc(),
diag::warn_attribute_dllexport_explicit_instantiation_def);
if (PrevDecl->hasAttr<DLLExportAttr>()) {
Diag(AL.getLoc(), diag::warn_attr_dllexport_explicit_inst_def);
} else {
Diag(AL.getLoc(),
diag::warn_attr_dllexport_explicit_inst_def_mismatch);
Diag(PrevDecl->getLocation(), diag::note_prev_decl_missing_dllexport);
}
break;
}
}
}

if (TSK == TSK_ExplicitInstantiationDefinition && PrevDecl &&
!Context.getTargetInfo().getTriple().isWindowsGNUEnvironment() &&
llvm::none_of(Attr, [](const ParsedAttr &AL) {
return AL.getKind() == ParsedAttr::AT_DLLExport;
})) {
if (const auto *DEA = PrevDecl->getAttr<DLLExportOnDeclAttr>()) {
Diag(TemplateLoc, diag::warn_dllexport_on_decl_ignored);
Diag(DEA->getLoc(), diag::note_dllexport_on_decl);
}
}

if (CheckExplicitInstantiation(*this, ClassTemplate, TemplateNameLoc,
SS.isSet(), TSK))
return true;
Expand Down
19 changes: 19 additions & 0 deletions clang/test/SemaCXX/dllexport-explicit-instantiation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// RUN: %clang_cc1 -triple x86_64-win32 -fsyntax-only -fms-extensions -verify=win32,win32-pedantic -DMS %s -Wdllexport-explicit-instantiation
// RUN: %clang_cc1 -triple x86_64-win32 -fsyntax-only -fms-extensions -verify=win32 -DMS %s -Wno-dllexport-explicit-instantiation
// RUN: %clang_cc1 -triple x86_64-mingw32 -fsyntax-only -fms-extensions -verify=mingw,mingw-pedantic -DMS %s -Wdllexport-explicit-instantiation
// RUN: %clang_cc1 -triple x86_64-mingw32 -fsyntax-only -fms-extensions -verify=mingw -DMS %s -Wno-dllexport-explicit-instantiation

template <class>
class S {};

extern template class __declspec(dllexport) S<short>; // win32-pedantic-warning {{explicit instantiation declaration should not be 'dllexport'}} \
win32-pedantic-note {{attribute is here}}
template class __declspec(dllexport) S<short>; // mingw-pedantic-warning {{'dllexport' attribute ignored on explicit instantiation definition}}

extern template class __declspec(dllexport) S<int>; // win32-pedantic-warning {{explicit instantiation declaration should not be 'dllexport'}} \
win32-pedantic-note {{attribute is here}} \
win32-note {{'dllexport' attribute on the declaration is ignored}}
template class S<int>; // win32-warning {{explicit instantiation definition is not exported without 'dllexport'}}

extern template class S<long>; // mingw-note {{'dllexport' attribute is missing on previous declaration}}
template class __declspec(dllexport) S<long>; // mingw-warning {{'dllexport' attribute ignored on explicit instantiation definition}}
3 changes: 3 additions & 0 deletions clang/test/SemaCXX/dllexport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,9 @@ template struct ExplicitlyInstantiatedTemplate<int>;
template <typename T> struct ExplicitlyExportInstantiatedTemplate { void func() {} };
template struct __declspec(dllexport) ExplicitlyExportInstantiatedTemplate<int>;
template <typename T> struct ExplicitlyExportDeclaredInstantiatedTemplate { void func() {} };
#ifdef GNU
Copy link
Collaborator

@erichkeane erichkeane Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Urgh, I hate macros for this purpose, we have the verify= flag for these things, and this file is awful about this...

Could you instead of 'making it worse' (even though only slightly), change it to -verify=expected,gnu and change these to //gnu-note@... instead?

I realize the rest of the file is still badly done, but it is at least... a step in the right direction.

// expected-note@+2 {{attribute is missing}}
#endif
extern template struct ExplicitlyExportDeclaredInstantiatedTemplate<int>;
#if not defined(MS) && not defined (WI) && not defined(PS)
// expected-warning@+2{{'dllexport' attribute ignored on explicit instantiation definition}}
Expand Down