Customize warnings for missing built-in type

[jdoerfert]

Summary

If we detect a built-in declaration for which we cannot derive a type
matching the pattern in the Builtins.def file, we currently emit a
warning that the respective header is needed. However, this is not
necessarily the behavior we want as it has no connection to the location
of the declaration (which can actually be in the header in question).
Instead, this warning is generated

• if we could not build the type for the pattern on file (for some reason). Here we should make the reason explicit. The actual problem is otherwise circumvented as the warning is misleading, see [0] for an example.
• if we could not build the type for the pattern because we do not have a type on record, possible since D55483, we should not emit any warning. See [1] for a legitimate problem.

This patch address both cases. For the "setjmp" family a new warning is
introduced and for built-ins without type on record, so far
"pthread_create", we do not emit the warning anymore.

Also see: PR40692

[0] https://lkml.org/lkml/2019/1/11/718
[1] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=235583

[aaron.ballman]

If I'm following along properly, it sounds like we want to disable this warning largely because it can appear in header files attempting to declare the functions in question -- but I wonder why those diagnostics are happening in the first place. It seems like the warning is still useful when it triggers outside of that situation, no?

clang/lib/Sema/SemaDecl.cpp

1974 ↗

(On Diff #186380)

Generally -> Generally,

[bcain]

I reported PR40692. I just tried this patch on our local build where we saw the failure on an RTOS implementing pthreads. Unfortunately with this patch I encountered an (unrelated) assertion. So this fix was inconclusive for me (for now). I will follow up but if this fix makes sense then you don't need to wait for my test results.

[jdoerfert]

In D58091#1396382, @aaron.ballman wrote:

If I'm following along properly, it sounds like we want to disable this warning largely because it can appear in header files attempting to declare the functions in question.

That is the situation that exposed the problem, yes.

• but I wonder why those diagnostics are happening in the first place. It seems like the warning is still useful when it triggers outside of that situation, no?

The underlying conceptual problem, which I didn't know when I added GE_Missing_type, is that this has _nothing_ to do with the location of the declaration. We say, include the header X.h, if we were not able to build a type for recognized built-in Y that should be declared in X.h. However, we should report _why_ we could not build the type instead. For built-ins we do not have a type on record (GE_Missing_type), this is always, so no warning for now. For the ones that we only fail to build a type because some requirement is missing, we should report that, at least when we are in the respective header. I don't have a perfect solution of what to do actually.

I could check if the declaration is (probably) in the respective header so we can switch between warnings?

[jdoerfert]

In D58091#1396393, @bcain wrote:

I reported PR40692. I just tried this patch on our local build where we saw the failure on an RTOS implementing pthreads. Unfortunately with this patch I encountered an (unrelated) assertion. So this fix was inconclusive for me (for now). I will follow up but if this fix makes sense then you don't need to wait for my test results.

Are you sure it is unrelated?

[aaron.ballman]

In D58091#1396397, @jdoerfert wrote:

In D58091#1396382, @aaron.ballman wrote:

• but I wonder why those diagnostics are happening in the first place. It seems like the warning is still useful when it triggers outside of that situation, no?

The underlying conceptual problem, which I didn't know when I added GE_Missing_type, is that this has _nothing_ to do with the location of the declaration. We say, include the header X.h, if we were not able to build a type for recognized built-in Y that should be declared in X.h. However, we should report _why_ we could not build the type instead. For built-ins we do not have a type on record (GE_Missing_type), this is always, so no warning for now. For the ones that we only fail to build a type because some requirement is missing, we should report that, at least when we are in the respective header. I don't have a perfect solution of what to do actually.

I could check if the declaration is (probably) in the respective header so we can switch between warnings?

That's kind of what I was wondering, but I deal with builtins so infrequently that my expectations may be wrong here. If a user declares a builtin with a conflicting type outside of a header file, that seems like we'd want to warn the user about right? But this seems to remove that warning, at least in the case of test/Sema/implicit-builtin-decl.c:71. Or do I misunderstand the situation causing the warning to trigger?

[bcain]

Are you sure it is unrelated?

Very nearly positive, I applied the patch to our downstream trunk and the assertion is encountered in a target-specific pass.

[jdoerfert]

In D58091#1396437, @aaron.ballman wrote:

In D58091#1396397, @jdoerfert wrote:

In D58091#1396382, @aaron.ballman wrote:

• but I wonder why those diagnostics are happening in the first place. It seems like the warning is still useful when it triggers outside of that situation, no?

The underlying conceptual problem, which I didn't know when I added GE_Missing_type, is that this has _nothing_ to do with the location of the declaration. We say, include the header X.h, if we were not able to build a type for recognized built-in Y that should be declared in X.h. However, we should report _why_ we could not build the type instead. For built-ins we do not have a type on record (GE_Missing_type), this is always, so no warning for now. For the ones that we only fail to build a type because some requirement is missing, we should report that, at least when we are in the respective header. I don't have a perfect solution of what to do actually.

I could check if the declaration is (probably) in the respective header so we can switch between warnings?

That's kind of what I was wondering, but I deal with builtins so infrequently that my expectations may be wrong here. If a user declares a builtin with a conflicting type outside of a header file, that seems like we'd want to warn the user about right? But this seems to remove that warning, at least in the case of test/Sema/implicit-builtin-decl.c:71. Or do I misunderstand the situation causing the warning to trigger?

After this, we should still warn for all builtins for which we have an expected type on record.

I added the clang/test/Sema/builtin-setjmp.c test to check for this situation. Here, setjmp is declared outside of the header (but it actually doesn't matter as I mentioned in the above comment). If you declare it without defining jmp_buf first, that is what the kernel ppl did, you will get a warning that states jmp_buf is unknown and we require it for the declaration of setjmp/longjmp/... (line 5). If you define jmp_buf and then declare setjmp with a conflicting type, that is not T setjmp(jmp_buf), you will see the incompatible redeclaration warning (line 8). Does that make sense?

[rsmith]

clang/include/clang/Basic/DiagnosticSemaKinds.td

591 ↗

(On Diff #186380)

We only require a declaration of jmp_buf, not a definition. And please retain the word "function" after "built-in".

clang/lib/Sema/SemaDecl.cpp

1971 ↗

(On Diff #186380)

It'd be nice to produce note_include_header_or_declare here. (Ideally, that note should be suppressed if we're transitively in a header with the right name already, but I think it'll be clear enough what's wrong even if we produce the note unconditionally.)

clang/test/Sema/builtin-setjmp.c

4–10 ↗

(On Diff #186380)

You don't need custom -verify prefixes for this (-verify respects #ifdefs); just use expected.

[jyknight]

I think this warning (-Wbuiltin-requires-header) doesn't really make sense as its own warning.

We already have two related (on-by-default) warnings.

For declarations:

test.c:1:6: warning: incompatible redeclaration of library function 'exit' [-Wincompatible-library-redeclaration]
long exit(char *);
     ^
test.c:1:6: note: 'exit' is a builtin with type 'void (int) __attribute__((noreturn))'

And for uses:

test2.c:1:13: warning: implicitly declaring library function 'exit' with type 'void (int) __attribute__((noreturn))' [-Wimplicit-function-declaration]
int foo() { exit(0); }
            ^
test2.c:1:13: note: include the header <stdlib.h> or explicitly provide a declaration for 'exit'

I think for a declaration, if we cannot construct the appropriate type, we should be treating all declarations as an incompatible redeclaration, and explain why in an attached note, like:

warning: incompatible redeclaration of library function 'exit' [-Wincompatible-library-redeclaration]
note: missing declaration of type 'jmp_buf' for argument 1 of standard function signature.

For a usage, we could emit something like:

warning: implicit declaration of library function 'setjmp' [-Wimplicit-function-declaration]
note: missing declaration of type 'jmp_buf' for argument 1.
note: include the header <setjmp.h> or explicitly provide a declaration for 'setjmp'

[bcain]

I can confirm that this fix is effective at addressing the problem we experienced.

[jdoerfert]

Address comments

[jdoerfert]

I did address the comments but I will wait until I hear back on the "warning vs warning + note question".

In D58091#1397586, @jyknight wrote:

I think this warning (-Wbuiltin-requires-header) doesn't really make sense as its own warning.

[...]

I think for a declaration, if we cannot construct the appropriate type, we should be treating all declarations as an incompatible redeclaration, and explain why in an attached note, like:

warning: incompatible redeclaration of library function 'exit' [-Wincompatible-library-redeclaration]
note: missing declaration of type 'jmp_buf' for argument 1 of standard function signature.

For a usage, we could emit something like:

warning: implicit declaration of library function 'setjmp' [-Wimplicit-function-declaration]
note: missing declaration of type 'jmp_buf' for argument 1.
note: include the header <setjmp.h> or explicitly provide a declaration for 'setjmp'

I do not have strong feelings about this, either way is fine with me. However, I lack the
clang expertise to make such a change happen anytime soon which makes this patch
(with actual fix for the warning on pthread_create) my prefered first step.

clang/lib/Sema/SemaDecl.cpp

1971 ↗

(On Diff #186380)

I did add the "include the header" part in the warning now. Does that make sense and address your issue or do you think we should have a separate note?

[bcain]

clang/include/clang/Basic/DiagnosticSemaKinds.td

592 ↗

(On Diff #188238)

"proived" should be "provided", I think?

[jdoerfert]

Fix typo

[bcain]

In D58091#1409584, @jdoerfert wrote:

I did address the comments but I will wait until I hear back on the "warning vs warning + note question".

...

I do not have strong feelings about this, either way is fine with me. However, I lack the
clang expertise to make such a change happen anytime soon which makes this patch
(with actual fix for the warning on pthread_create) my prefered first step.

FWIW, I'm satisfied with the fix as proposed here for now and I wouldn't be opposed to following up with an improvement over other warnings in lieu of this warning.

@jyknight -- James, (or others) care to weigh in on this proposal?

[bcain]

In D58091#1397586, @jyknight wrote:

I think this warning (-Wbuiltin-requires-header) doesn't really make sense as its own warning.

We already have two related (on-by-default) warnings.

...

In D58091#1414250, @bcain wrote:

...

FWIW, I'm satisfied with the fix as proposed here for now and I wouldn't be opposed to following up with an improvement over other warnings in lieu of this warning.

@jyknight -- James, (or others) care to weigh in on this proposal?

@jdoerfert and @jyknight -- let's please un-stall this review.

James: is this change acceptable or unacceptable as-is? Could we follow up with a change that removed this warning?

Johannes: if it's acceptable to James, let's please submit this change.

[bcain]

@jdoerfert @jyknight @lebedev.ri @rsmith @aaron.ballman

This will appear as a 9.0 regression, right? Let's please consider a way to address it for 9.0 before it's released.

[jdoerfert]

Fix spelling in tests

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 
 /// Built-in functions.
 def ext_implicit_lib_function_decl : ExtWarn<
   "implicitly declaring library function '%0' with type %1">,
   InGroup<ImplicitFunctionDeclare>;
 def note_include_header_or_declare : Note<
   "include the header <%0> or explicitly provide a declaration for '%1'">;
 def note_previous_builtin_declaration : Note<"%0 is a builtin with type %1">;
+def warn_implicit_decl_no_jmp_buf
+    : Warning<"declaration of built-in function '%0' requires the declaration"
+    " of the 'jmp_buf' type, commonly provided in the header <setjmp.h>.">,
+      InGroup<DiagGroup<"incomplete-setjmp-declaration">>;
 def warn_implicit_decl_requires_sysheader : Warning<
   "declaration of built-in function '%1' requires inclusion of the header <%0>">,
   InGroup<BuiltinRequiresHeader>;
 def warn_redecl_library_builtin : Warning<
   "incompatible redeclaration of library function %0">,
   InGroup<DiagGroup<"incompatible-library-redeclaration">>;
 def err_builtin_definition : Error<"definition of builtin function %0">;
 def err_builtin_redeclare : Error<"cannot redeclare builtin function %0">;

cfe/trunk/lib/Sema/SemaDecl.cpp

 NamedDecl *Sema::LazilyCreateBuiltin(IdentifierInfo *II, unsigned ID,
                                      Scope *S, bool ForRedeclaration,
                                      SourceLocation Loc) {
   LookupPredefedObjCSuperType(*this, S, II);
 
   ASTContext::GetBuiltinTypeError Error;
   QualType R = Context.GetBuiltinType(ID, Error);
   if (Error) {
-    if (ForRedeclaration)
-      Diag(Loc, diag::warn_implicit_decl_requires_sysheader)
-          << getHeaderName(Context.BuiltinInfo, ID, Error)
-          << Context.BuiltinInfo.getName(ID);
+    if (!ForRedeclaration)
+      return nullptr;
+
+    // If we have a builtin without an associated type we should not emit a
+    // warning when we were not able to find a type for it.
+    if (Error == ASTContext::GE_Missing_type)
+      return nullptr;
+
+    // If we could not find a type for setjmp it is because the jmp_buf type was
+    // not defined prior to the setjmp declaration.
+    if (Error == ASTContext::GE_Missing_setjmp) {
+      Diag(Loc, diag::warn_implicit_decl_no_jmp_buf)
+          << Context.BuiltinInfo.getName(ID);
+      return nullptr;
+    }
+
+    // Generally, we emit a warning that the declaration requires the
+    // appropriate header.
     return nullptr;
   }
 
   if (!ForRedeclaration &&
       (Context.BuiltinInfo.isPredefinedLibFunction(ID) ||
        Context.BuiltinInfo.isHeaderDependentFunction(ID))) {
     Diag(Loc, diag::ext_implicit_lib_function_decl)
         << Context.BuiltinInfo.getName(ID) << R;

cfe/trunk/test/Analysis/retain-release.m

 // RUN: rm -f %t.objc.plist %t.objcpp.plist
 // RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10\
 // RUN:     -analyzer-checker=core,osx.coreFoundation.CFRetainRelease\
 // RUN:     -analyzer-checker=osx.cocoa.ClassRelease,osx.cocoa.RetainCount\
-// RUN:     -analyzer-checker=debug.ExprInspection -fblocks -verify=expected,C %s\
+// RUN:     -analyzer-checker=debug.ExprInspection -fblocks -verify %s\
 // RUN:     -Wno-objc-root-class -analyzer-output=plist -o %t.objc.plist
 // RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10\
 // RUN:     -analyzer-checker=core,osx.coreFoundation.CFRetainRelease\
 // RUN:     -analyzer-checker=osx.cocoa.ClassRelease,osx.cocoa.RetainCount\
 // RUN:     -analyzer-checker=debug.ExprInspection -fblocks -verify %s\
 // RUN:     -Wno-objc-root-class -analyzer-output=plist -o %t.objcpp.plist\
 // RUN:     -x objective-c++ -std=gnu++98
 // RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10\

 struct _opaque_pthread_attr_t {};
 typedef struct _opaque_pthread_t *__darwin_pthread_t;
 typedef struct _opaque_pthread_attr_t __darwin_pthread_attr_t;
 typedef __darwin_pthread_t pthread_t;
 typedef __darwin_pthread_attr_t pthread_attr_t;
 typedef unsigned long __darwin_pthread_key_t;
 typedef __darwin_pthread_key_t pthread_key_t;
 
-int pthread_create(pthread_t *, const pthread_attr_t *,  // C-warning{{declaration of built-in function 'pthread_create' requires inclusion of the header <pthread.h>}}
+int pthread_create(pthread_t *, const pthread_attr_t *,
                    void *(*)(void *), void *);
 
 int pthread_setspecific(pthread_key_t key, const void *value);
 
 void *rdar_7299394_start_routine(void *p) {
   [((id) p) release];
   return 0;
 }

cfe/trunk/test/Sema/builtin-setjmp.c

+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify -DNO_JMP_BUF %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
+#ifdef NO_JMP_BUF
+extern long setjmp(long *);   // expected-warning {{declaration of built-in function 'setjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header <setjmp.h>.}}
+#else
+typedef long jmp_buf;
+extern int setjmp(char);      // expected-warning@8 {{incompatible redeclaration of library function 'setjmp'}}
+                              // expected-note@8    {{'setjmp' is a builtin with type 'int (jmp_buf)' (aka 'int (long)')}}
+#endif

cfe/trunk/test/Sema/implicit-builtin-decl.c

 int
 main(int argc, char *argv[])
 {
   snprintf();
 }
 
 void snprintf() { }
 
-// PR8316
-void longjmp(); // expected-warning{{declaration of built-in function 'longjmp' requires inclusion of the header <setjmp.h>}}
+// PR8316 & PR40692
+void longjmp(); // expected-warning{{declaration of built-in function 'longjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header <setjmp.h>.}}
 
 extern float fmaxf(float, float);
 
 struct __jmp_buf_tag {};
-void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration of built-in function 'sigsetjmp' requires inclusion of the header <setjmp.h>}}
+void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header <setjmp.h>.}}
 
 // CHECK:     FunctionDecl {{.*}} <line:[[@LINE-2]]:1, col:44> col:6 sigsetjmp '
 // CHECK-NOT: FunctionDecl
 // CHECK:     ReturnsTwiceAttr {{.*}} <{{.*}}> Implicit
+
+// PR40692
+void pthread_create(); // no warning expected