Add macro to suppress -Wunnecessary-virtual-specifier #139614
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-ir Author: Devon Loehr (DKLoehr) ChangesFollowup to #138741. This adds the requested macro to silence It also cleans up any remaining instances of the warning, allowing us to stop disabling it when we build LLVM. Full diff: https://github.com/llvm/llvm-project/pull/139614.diff 19 Files Affected:
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index 3faf63e395a08..2db97fa67ac2c 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -172,7 +172,7 @@ class PragmaCommentDecl final
PragmaMSCommentKind CommentKind)
: Decl(PragmaComment, TU, CommentLoc), CommentKind(CommentKind) {}
- virtual void anchor();
+ LLVM_DECLARE_VIRTUAL_ANCHOR_FUNCTION();
public:
static PragmaCommentDecl *Create(const ASTContext &C, TranslationUnitDecl *DC,
@@ -206,7 +206,7 @@ class PragmaDetectMismatchDecl final
size_t ValueStart)
: Decl(PragmaDetectMismatch, TU, Loc), ValueStart(ValueStart) {}
- virtual void anchor();
+ LLVM_DECLARE_VIRTUAL_ANCHOR_FUNCTION();
public:
static PragmaDetectMismatchDecl *Create(const ASTContext &C,
@@ -5031,7 +5031,7 @@ class ImportDecl final : public Decl,
/// export void foo();
/// \endcode
class ExportDecl final : public Decl, public DeclContext {
- virtual void anchor();
+ LLVM_DECLARE_VIRTUAL_ANCHOR_FUNCTION();
private:
friend class ASTDeclReader;
diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
index b7980137002aa..2ae58077466f5 100644
--- a/clang/include/clang/AST/DeclCXX.h
+++ b/clang/include/clang/AST/DeclCXX.h
@@ -3296,7 +3296,7 @@ class LifetimeExtendedTemporaryDecl final
mutable APValue *Value = nullptr;
- virtual void anchor();
+ LLVM_DECLARE_VIRTUAL_ANCHOR_FUNCTION();
LifetimeExtendedTemporaryDecl(Expr *Temp, ValueDecl *EDecl, unsigned Mangling)
: Decl(Decl::LifetimeExtendedTemporary, EDecl->getDeclContext(),
diff --git a/clang/include/clang/AST/DeclFriend.h b/clang/include/clang/AST/DeclFriend.h
index 1578580c89cd8..3435f5933314b 100644
--- a/clang/include/clang/AST/DeclFriend.h
+++ b/clang/include/clang/AST/DeclFriend.h
@@ -52,7 +52,7 @@ class ASTContext;
class FriendDecl final
: public Decl,
private llvm::TrailingObjects<FriendDecl, TemplateParameterList *> {
- virtual void anchor();
+ LLVM_DECLARE_VIRTUAL_ANCHOR_FUNCTION();
public:
using FriendUnion = llvm::PointerUnion<NamedDecl *, TypeSourceInfo *>;
diff --git a/clang/include/clang/AST/DeclOpenMP.h b/clang/include/clang/AST/DeclOpenMP.h
index cf383889c0ad9..2d07f9d9f5d8c 100644
--- a/clang/include/clang/AST/DeclOpenMP.h
+++ b/clang/include/clang/AST/DeclOpenMP.h
@@ -110,7 +110,7 @@ template <typename U> class OMPDeclarativeDirective : public U {
class OMPThreadPrivateDecl final : public OMPDeclarativeDirective<Decl> {
friend class OMPDeclarativeDirective<Decl>;
- virtual void anchor();
+ LLVM_DECLARE_VIRTUAL_ANCHOR_FUNCTION();
OMPThreadPrivateDecl(DeclContext *DC = nullptr,
SourceLocation L = SourceLocation())
@@ -418,7 +418,7 @@ class OMPRequiresDecl final : public OMPDeclarativeDirective<Decl> {
friend class OMPDeclarativeDirective<Decl>;
friend class ASTDeclReader;
- virtual void anchor();
+ LLVM_DECLARE_VIRTUAL_ANCHOR_FUNCTION();
OMPRequiresDecl(DeclContext *DC, SourceLocation L)
: OMPDeclarativeDirective<Decl>(OMPRequires, DC, L) {}
@@ -475,7 +475,7 @@ class OMPAllocateDecl final : public OMPDeclarativeDirective<Decl> {
friend class OMPDeclarativeDirective<Decl>;
friend class ASTDeclReader;
- virtual void anchor();
+ LLVM_DECLARE_VIRTUAL_ANCHOR_FUNCTION();
OMPAllocateDecl(DeclContext *DC, SourceLocation L)
: OMPDeclarativeDirective<Decl>(OMPAllocate, DC, L) {}
diff --git a/clang/include/clang/Driver/Action.h b/clang/include/clang/Driver/Action.h
index 92bb19314e3d6..7aecfd886adb8 100644
--- a/clang/include/clang/Driver/Action.h
+++ b/clang/include/clang/Driver/Action.h
@@ -267,7 +267,7 @@ class BindArchAction : public Action {
/// programming model implementation needs and propagates the offloading kind to
/// its dependences.
class OffloadAction final : public Action {
- virtual void anchor();
+ LLVM_DECLARE_VIRTUAL_ANCHOR_FUNCTION();
public:
/// Type used to communicate device actions. It associates bound architecture,
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 6ea7ee281e14d..b91c2a504f0a7 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -895,7 +895,7 @@ class Sema final : public SemaBase {
/// with a vtable when the vtable is emitted. Sema is final and not
/// polymorphic, but the debug info size savings are so significant that it is
/// worth adding a vtable just to take advantage of this optimization.
- virtual void anchor();
+ LLVM_DECLARE_VIRTUAL_ANCHOR_FUNCTION();
const LangOptions &getLangOpts() const { return LangOpts; }
OpenCLOptions &getOpenCLOptions() { return OpenCLFeatures; }
diff --git a/clang/lib/AST/ByteCode/InterpFrame.h b/clang/lib/AST/ByteCode/InterpFrame.h
index 360e6bff12327..cfebe936cd468 100644
--- a/clang/lib/AST/ByteCode/InterpFrame.h
+++ b/clang/lib/AST/ByteCode/InterpFrame.h
@@ -119,7 +119,7 @@ class InterpFrame final : public Frame {
CodePtr getRetPC() const { return RetPC; }
/// Map a location to a source.
- virtual SourceInfo getSource(CodePtr PC) const;
+ SourceInfo getSource(CodePtr PC) const;
const Expr *getExpr(CodePtr PC) const;
SourceLocation getLocation(CodePtr PC) const;
SourceRange getRange(CodePtr PC) const;
diff --git a/clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp b/clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
index aa97422d54ede..6aca31f8b4427 100644
--- a/clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
+++ b/clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
@@ -323,7 +323,7 @@ class CheckVarsEscapingDeclContext final
CheckVarsEscapingDeclContext(CodeGenFunction &CGF,
ArrayRef<const ValueDecl *> TeamsReductions)
: CGF(CGF), EscapedDecls(llvm::from_range, TeamsReductions) {}
- virtual ~CheckVarsEscapingDeclContext() = default;
+ ~CheckVarsEscapingDeclContext() = default;
void VisitDeclStmt(const DeclStmt *S) {
if (!S)
return;
diff --git a/clang/lib/CodeGen/CGStmtOpenMP.cpp b/clang/lib/CodeGen/CGStmtOpenMP.cpp
index 803c7ed37635e..89a79a7579e03 100644
--- a/clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ b/clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -4783,7 +4783,7 @@ class CheckVarsEscapingUntiedTaskDeclContext final
public:
explicit CheckVarsEscapingUntiedTaskDeclContext() = default;
- virtual ~CheckVarsEscapingUntiedTaskDeclContext() = default;
+ ~CheckVarsEscapingUntiedTaskDeclContext() = default;
void VisitDeclStmt(const DeclStmt *S) {
if (!S)
return;
diff --git a/clang/lib/Driver/ToolChains/Hexagon.h b/clang/lib/Driver/ToolChains/Hexagon.h
index e35a224dced41..033d9b48cae10 100644
--- a/clang/lib/Driver/ToolChains/Hexagon.h
+++ b/clang/lib/Driver/ToolChains/Hexagon.h
@@ -42,8 +42,8 @@ class LLVM_LIBRARY_VISIBILITY Linker final : public Tool {
bool hasIntegratedCPP() const override { return false; }
bool isLinkJob() const override { return true; }
- virtual void RenderExtraToolArgs(const JobAction &JA,
- llvm::opt::ArgStringList &CmdArgs) const;
+ void RenderExtraToolArgs(const JobAction &JA,
+ llvm::opt::ArgStringList &CmdArgs) const;
void ConstructJob(Compilation &C, const JobAction &JA,
const InputInfo &Output, const InputInfoList &Inputs,
const llvm::opt::ArgList &TCArgs,
diff --git a/llvm/cmake/modules/HandleLLVMOptions.cmake b/llvm/cmake/modules/HandleLLVMOptions.cmake
index c427a65ee030c..8b3303fe9f3d2 100644
--- a/llvm/cmake/modules/HandleLLVMOptions.cmake
+++ b/llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -882,11 +882,6 @@ if (LLVM_ENABLE_WARNINGS AND (LLVM_COMPILER_IS_GCC_COMPATIBLE OR CLANG_CL))
# The LLVM libraries have no stable C++ API, so -Wnoexcept-type is not useful.
append("-Wno-noexcept-type" CMAKE_CXX_FLAGS)
- # LLVM has a policy of including virtual "anchor" functions to control
- # where the vtable is emitted. In `final` classes, these are exactly what
- # this warning detects: unnecessary virtual methods.
- add_flag_if_supported("-Wno-unnecessary-virtual-specifier" CXX_SUPPORTS_UNNECESSARY_VIRTUAL_FLAG)
-
if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
append("-Wnon-virtual-dtor" CMAKE_CXX_FLAGS)
endif()
diff --git a/llvm/include/llvm/Analysis/InstSimplifyFolder.h b/llvm/include/llvm/Analysis/InstSimplifyFolder.h
index d4ae4dcc918cf..fbf6764212167 100644
--- a/llvm/include/llvm/Analysis/InstSimplifyFolder.h
+++ b/llvm/include/llvm/Analysis/InstSimplifyFolder.h
@@ -36,7 +36,7 @@ class InstSimplifyFolder final : public IRBuilderFolder {
TargetFolder ConstFolder;
SimplifyQuery SQ;
- virtual void anchor();
+ LLVM_DECLARE_VIRTUAL_ANCHOR_FUNCTION();
public:
explicit InstSimplifyFolder(const DataLayout &DL) : ConstFolder(DL), SQ(DL) {}
diff --git a/llvm/include/llvm/Analysis/TargetFolder.h b/llvm/include/llvm/Analysis/TargetFolder.h
index 4c78211b5c935..57a883e16dd25 100644
--- a/llvm/include/llvm/Analysis/TargetFolder.h
+++ b/llvm/include/llvm/Analysis/TargetFolder.h
@@ -39,7 +39,7 @@ class TargetFolder final : public IRBuilderFolder {
return ConstantFoldConstant(C, DL);
}
- virtual void anchor();
+ LLVM_DECLARE_VIRTUAL_ANCHOR_FUNCTION();
public:
explicit TargetFolder(const DataLayout &DL) : DL(DL) {}
diff --git a/llvm/include/llvm/IR/ConstantFolder.h b/llvm/include/llvm/IR/ConstantFolder.h
index a75cdf97f6ed3..a1d9a8faf69e1 100644
--- a/llvm/include/llvm/IR/ConstantFolder.h
+++ b/llvm/include/llvm/IR/ConstantFolder.h
@@ -28,7 +28,7 @@ namespace llvm {
/// ConstantFolder - Create constants with minimum, target independent, folding.
class ConstantFolder final : public IRBuilderFolder {
- virtual void anchor();
+ LLVM_DECLARE_VIRTUAL_ANCHOR_FUNCTION();
public:
explicit ConstantFolder() = default;
diff --git a/llvm/include/llvm/IR/NoFolder.h b/llvm/include/llvm/IR/NoFolder.h
index c4631a9ba1cbf..2a2318dfd7863 100644
--- a/llvm/include/llvm/IR/NoFolder.h
+++ b/llvm/include/llvm/IR/NoFolder.h
@@ -33,7 +33,7 @@ namespace llvm {
/// NoFolder - Create "constants" (actually, instructions) with no folding.
class NoFolder final : public IRBuilderFolder {
- virtual void anchor();
+ LLVM_DECLARE_VIRTUAL_ANCHOR_FUNCTION();
public:
explicit NoFolder() = default;
diff --git a/llvm/include/llvm/Support/Compiler.h b/llvm/include/llvm/Support/Compiler.h
index 4864071ed87a8..576fa55d8063c 100644
--- a/llvm/include/llvm/Support/Compiler.h
+++ b/llvm/include/llvm/Support/Compiler.h
@@ -710,4 +710,14 @@ void AnnotateIgnoreWritesEnd(const char *file, int line);
#define LLVM_PREFERRED_TYPE(T)
#endif
+/// \macro LLVM_VIRTUAL_ANCHOR_FUNCTION
+/// This macro is used to adhere to LLVM's policy that each class with a vtable
+/// must have at least one out-of-line virtual function. This macro allows us
+/// to declare such a function in `final` classes without triggering a warning.
+#define LLVM_DECLARE_VIRTUAL_ANCHOR_FUNCTION() \
+ _Pragma("clang diagnostic push") \
+ _Pragma("clang diagnostic ignored \"-Wunnecessary-virtual-specifier\"") \
+ virtual void anchor()\
+ _Pragma("clang diagnostic pop")
+
#endif
diff --git a/llvm/include/llvm/Transforms/Scalar/GVNExpression.h b/llvm/include/llvm/Transforms/Scalar/GVNExpression.h
index 50cd6ceb9fc0d..1629aacbae45c 100644
--- a/llvm/include/llvm/Transforms/Scalar/GVNExpression.h
+++ b/llvm/include/llvm/Transforms/Scalar/GVNExpression.h
@@ -452,7 +452,7 @@ class AggregateValueExpression final : public BasicExpression {
IntOperands[NumIntOperands++] = IntOperand;
}
- virtual void allocateIntOperands(BumpPtrAllocator &Allocator) {
+ void allocateIntOperands(BumpPtrAllocator &Allocator) {
assert(!IntOperands && "Operands already allocated");
IntOperands = Allocator.Allocate<unsigned>(MaxIntOperands);
}
diff --git a/llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp b/llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp
index eb768ed9ad5a1..a67c3762df78f 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp
@@ -1212,7 +1212,7 @@ class FatPtrConstMaterializer final : public ValueMaterializer {
ValueToValueMapTy &UnderlyingMap)
: TypeMap(TypeMap),
InternalMapper(UnderlyingMap, RF_None, TypeMap, this) {}
- virtual ~FatPtrConstMaterializer() = default;
+ ~FatPtrConstMaterializer() = default;
Value *materialize(Value *V) override;
};
diff --git a/llvm/lib/Target/X86/X86InstrInfo.h b/llvm/lib/Target/X86/X86InstrInfo.h
index e53f2566dd892..9dc5f4b0e086e 100644
--- a/llvm/lib/Target/X86/X86InstrInfo.h
+++ b/llvm/lib/Target/X86/X86InstrInfo.h
@@ -225,7 +225,7 @@ class X86InstrInfo final : public X86GenInstrInfo {
X86Subtarget &Subtarget;
const X86RegisterInfo RI;
- virtual void anchor();
+ LLVM_DECLARE_VIRTUAL_ANCHOR_FUNCTION();
bool analyzeBranchImpl(MachineBasicBlock &MBB, MachineBasicBlock *&TBB,
MachineBasicBlock *&FBB,
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I'm slightly confused about the direction here. Either Or we think no one else does this anchor business, and in that case... why are we doing it? |
|
I've only had ad-hoc discussions on the matter, but everyone I've talked to seems to fall in the latter camp. As mentioned in #138741, I didn't see any other project with a similar policy among chromium and all of its dependencies. I'd have no problem if we decided to do away with it altogether, but I don't know what that involves. It's worth noting that the instance in Sema.h is even more unusual, per the comment above it. In the meantime, this gets us back to the status quo, and lets us use the warning to detect truly-accidental |
rnk
reviewed
May 14, 2025
Anchoring debug information out of a header and into a .cpp file can be a powerful optimization. I added a key method to Sema in 586f65d and it resulted in significant object file size savings (-17.0%). I didn't measure time, but these things are often visible in That said, nobody is maintaining the placement of these anchor methods. It's totally ad-hoc, and the gains can easily be erased by adding inline virtual methods ahead of the anchor. Also, vtable slots aren't free. They cost dynamic relocations and negatively impact startup time (yuck). Homing constructors and destructors in cpp files is another way to achieve similar ends, but those tend to matter more for inlining. Chromium has that style rule, and they removed (or are removing) it. |
Yes, these are just the ones that triggered the warning (i.e. those in |
rnk
approved these changes
May 15, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Followup to #138741.
This adds the requested macro to silence
-Wunnecessary-virtual-specifierwhen declaring virtual anchor functions infinalclasses, per LLVM policy.It also cleans up any remaining instances of the warning, allowing us to stop disabling it when we build LLVM.