clang should have a warning to disallow re-externs #109714
Conversation
clang should have a warning to disallow re-externs from the main C translation unit
for example, it would be helpful to warn programmers in this situation
```c
// file1.c
extern int func(int a, int b);
int some_func() {
func(1,2);
}
```
```c
// file2.c
int func(int a, int b, char *c, int d) {
// function body
}
```
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Jeffrey Crowell (jchcrsp) Changesclang should have a warning to disallow re-externs from the main C translation unit for example, it would be helpful to warn programmers in this situation // file1.c
extern int func(int a, int b);
int some_func() {
func(1,2);
}// file2.c
int func(int a, int b, char *c, int d) {
// function body
}Full diff: https://github.com/llvm/llvm-project/pull/109714.diff 6 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 7d81bdf827ea0c..9e8bd587094423 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1582,3 +1582,6 @@ def ExtractAPIMisuse : DiagGroup<"extractapi-misuse">;
// Warnings about using the non-standard extension having an explicit specialization
// with a storage class specifier.
def ExplicitSpecializationStorageClass : DiagGroup<"explicit-specialization-storage-class">;
+
+// Warnings about extern functions not in header files.
+def ExternalDeclaration : DiagGroup<"external-declaration">;
\ No newline at end of file
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index e4e04bff8b5120..16529dab1e245d 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -12614,4 +12614,10 @@ def err_acc_loop_spec_conflict
// AMDGCN builtins diagnostics
def err_amdgcn_global_load_lds_size_invalid_value : Error<"invalid size value">;
def note_amdgcn_global_load_lds_size_valid_value : Note<"size must be 1, 2, or 4">;
+
+// SemaExternWarning diagnostics
+def warn_extern_func_not_in_header : Warning<
+ "extern function '%0' declared in main file '%1' instead of a header">,
+ InGroup<ExternalDeclaration>, DefaultIgnore;
+
} // end of sema component.
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index e1c3a99cfa167e..76a9f4c84f47f7 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4390,6 +4390,15 @@ class Sema final : public SemaBase {
// linkage or not.
static bool mightHaveNonExternalLinkage(const DeclaratorDecl *FD);
+ ///@}
+ /// \name CheckExternFunction
+ ///@{
+public:
+ void CheckExternDecl(Decl *D);
+ void CheckDeferredExternDecls();
+
+private:
+ std::vector<FunctionDecl *> ExternFuncDecls;
///@}
//
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index 6d7a57d7b5a41a..4a4aefcd38e4c7 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -1177,6 +1177,8 @@ void Sema::ActOnEndOfTranslationUnit() {
if (PP.isCodeCompletionEnabled())
return;
+ CheckDeferredExternDecls();
+
// Complete translation units and modules define vtables and perform implicit
// instantiations. PCH files do not.
if (TUKind != TU_Prefix) {
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 31bf50a32a83c3..e2dbf9521aec6c 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -6058,6 +6058,10 @@ Decl *Sema::ActOnDeclarator(Scope *S, Declarator &D) {
Dcl && Dcl->getDeclContext()->isFileContext())
Dcl->setTopLevelDeclInObjCContainer();
+ if (Dcl) {
+ CheckExternDecl(Dcl);
+ }
+
if (!Bases.empty())
OpenMP().ActOnFinishedFunctionDefinitionInOpenMPDeclareVariantScope(Dcl,
Bases);
@@ -20354,3 +20358,33 @@ bool Sema::diagnoseConflictingFunctionEffect(
return false;
}
+
+void Sema::CheckExternDecl(Decl *D) {
+ if (FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
+ SourceLocation Loc = FD->getLocation();
+ SourceManager &SM = Context.getSourceManager();
+
+ // Only consider extern function declarations (not definitions) in the main
+ // file
+ if (FD->isExternC() && !FD->isImplicit() && !FD->getBuiltinID() &&
+ !FD->hasBody() && !FD->isThisDeclarationADefinition() &&
+ FD->isFirstDecl() && SM.isInMainFile(Loc)) {
+ // Defer the warning check until the end of the translation unit
+ ExternFuncDecls.push_back(FD);
+ }
+ }
+}
+
+void Sema::CheckDeferredExternDecls() {
+ SourceManager &SM = Context.getSourceManager();
+ for (FunctionDecl *FD : ExternFuncDecls) {
+ // Check if there's a definition in the same file
+ const FunctionDecl *Definition = FD->getDefinition();
+ if (!Definition || !SM.isInMainFile(Definition->getLocation())) {
+ SourceLocation Loc = FD->getLocation();
+ Diag(Loc, diag::warn_extern_func_not_in_header)
+ << FD->getName() << SM.getFilename(Loc);
+ }
+ }
+ ExternFuncDecls.clear();
+}
diff --git a/clang/test/Sema/warn-extern-func-not-in-header.c b/clang/test/Sema/warn-extern-func-not-in-header.c
new file mode 100644
index 00000000000000..62ba2a00400dd6
--- /dev/null
+++ b/clang/test/Sema/warn-extern-func-not-in-header.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wexternal-declaration %s
+
+extern int
+foo(int); // expected-warning{{extern function 'foo' declared in main file}}
+
+int bar(int);
+int bar(int x) {
+ return x + 1;
+}
+
+int main() {
+ return foo(42) + bar(10);
+}
\ No newline at end of file
|
Er, why would that be helpful? This is a valid pattern imo that I have seen here and there from time to time. Is there a specific reason why this is a bad idea? |
|
Oh wait, I see this is a case of the declaration and definition not matching. That is indeed bad, but there is no good way of distinguishing this from a declaration that actually matches the definition. You could put that declaration in a header, include it in one file, and then... not include it in another file and implement a completely different function with the same name. That would run into the same problem. I don’t know what the C term for this is, but in C++, situations like these are called IFNDR (ill-formed, no diagnostic required), because there is no good way of diagnosing this. This would have to be done at link time, and linkers don’t know a thing about the C type system. Simply diagnosing every extern declaration is not a solution for this and is going to lead to a lot of false positives—probably even mostly false positives because I’d expect that the people that actually do this take care to make sure the declaration and definition match. |
|
So yeah, in sum, I unfortunately don’t see this ever being a good idea |
I agree, it's not a complete solution, but it can be used to enforce calling only apis that are intended to be called/exported in headers. There are some codebases where programmers extern as a "hack" to call functions in an ugly way, and this warning would catch those scenarios. I think this warning doesn't belong as on by default, which is why I have added it to the |
|
Hmm, I personally still don’t think we want this warning, but maybe that’s just me. CC @AaronBallman @cor3ntin wdyt? |
|
Ping |
|
Ping fixed merge conflict |
|
I appreciate the idea, but I don't think it's a diagnostic we'd want to accept in Clang itself. We typically don't want to add new default-off diagnostics because there's plenty of evidence that they aren't enabled often enough to warrant having them. But also, this is really something that's better left to a tool other than the compiler. For example, Clang's static analyzer has cross-translation unit analysis support (https://clang.llvm.org/docs/analyzer/user-docs/CrossTranslationUnit.html) which seems like it would be a better fit. |
4 participants
clang should have a warning to disallow re-externs from the main C translation unit
for example, it would be helpful to warn programmers in this situation