Page MenuHomePhabricator

Customize warnings for missing built-in type
AcceptedPublic

Authored by jdoerfert on Feb 11 2019, 5:38 PM.

Details

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

Event Timeline

jdoerfert created this revision.Feb 11 2019, 5:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2019, 5:38 PM

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

Generally -> Generally,

bcain added a comment.Feb 13 2019, 8:21 AM

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.

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?

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?

  • 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 added a comment.Feb 13 2019, 9:31 AM

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.

  • 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 added inline comments.Feb 13 2019, 3:39 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
591

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

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
5–11

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

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 accepted this revision.Feb 20 2019, 7:08 AM

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

This revision is now accepted and ready to land.Feb 20 2019, 7:08 AM
jdoerfert marked 3 inline comments as done.

Address comments

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

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

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 added inline comments.Feb 25 2019, 12:51 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
592

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

jdoerfert marked an inline comment as done.

Fix typo

jdoerfert marked an inline comment as done.Feb 26 2019, 10:34 AM
bcain added a comment.Feb 28 2019, 2:40 PM

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?

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.

...

...

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.