Page MenuHomePhabricator

[clang] Specify type of pthread_create builtin
AcceptedPublic

Authored by jrtc27 on Feb 21 2019, 4:13 PM.

Details

Reviewers
rsmith
jdoerfert
Summary

Currently, pthread_create's type string is empty, so GetBuiltinType
fails and any header declaring it gives a -Wbuiltin-requires-header
warning, which gives a false-positive when including pthread.h itself
with -Wsystem-headers. By specifying its type, this false-positive goes
away and it behaves like every other builtin declared in system headers,
only warning when the declaration does not match the expected type.

This requires introducing a way to declare function pointer types in
builtin type strings, which requires refactoring GetBuiltinType to allow
recursive parsing of function types, expressed as a type string within
parentheses.

Event Timeline

jrtc27 created this revision.Feb 21 2019, 4:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2019, 4:13 PM

A separate point is whether it makes sense to be emitting this warning in the first place for GE_Missing_type. I'd argue that, if we don't know the type of the builtin, we should never emit the warning, since otherwise we always emit the warning even for legitimate instances, though that's not really important now we're not hitting that case any more.

srhines added inline comments.Mar 20 2019, 8:43 AM
clang/lib/AST/ASTContext.cpp
9574

I would just remove "of function" here, since it isn't correct when you are not decoding a function, and the two words don't make that much more meaningful even in the original case, so I don't think it justifies something like "of intrinsic/function".

probinson added a subscriber: probinson.

We've started running into this too in building the PS4 system. +jdoerfert who added pthread_create to the builtin list.

Looking at the patch, it seems straightforward enough although clearly needs clang-format-diff run over it.
I don't touch Clang that much so I'm reluctant to okay it myself.

We've started running into this too in building the PS4 system. +jdoerfert who added pthread_create to the builtin list.

Looking at the patch, it seems straightforward enough although clearly needs clang-format-diff run over it.
I don't touch Clang that much so I'm reluctant to okay it myself.

@probinson Thanks for making me aware of this patch.

A separate point is whether it makes sense to be emitting this warning in the first place for GE_Missing_type. I'd argue that, if we don't know the type of the builtin, we should never emit the warning

@jrtc27 I hope the immediate need for this is gone after D58091 was finally committed? Given that I caused this mess I can take a look at the patch if you are still interested in it, are you?

jrtc27 added a comment.Sep 8 2019, 8:14 AM

We've started running into this too in building the PS4 system. +jdoerfert who added pthread_create to the builtin list.

Looking at the patch, it seems straightforward enough although clearly needs clang-format-diff run over it.
I don't touch Clang that much so I'm reluctant to okay it myself.

@probinson Thanks for making me aware of this patch.

A separate point is whether it makes sense to be emitting this warning in the first place for GE_Missing_type. I'd argue that, if we don't know the type of the builtin, we should never emit the warning

@jrtc27 I hope the immediate need for this is gone after D58091 was finally committed? Given that I caused this mess I can take a look at the patch if you are still interested in it, are you?

Yes, now that it's not a warning if we don't provide a type this is no longer completely necessary. I would still be slightly in favour of this patch though as then we can detect dodgy pthread_create declarations, but it's not a big deal.

jdoerfert accepted this revision.Sep 8 2019, 12:01 PM

I like the patch and I think it is fine.

Small nits:
Could we have a test for " we can detect dodgy pthread_create declarations" and maybe pthread[_attr]_t?
There is an unresolved comment by @shrines.
@probinson: "it seems straightforward enough although clearly needs clang-format-diff run over it."

I'll accept this assuming the above points are easy to fix and given that no one expressed concerns but only positive comments were made.

clang/lib/AST/ASTContext.cpp
9239

I like the static helper function.

This revision is now accepted and ready to land.Sep 8 2019, 12:01 PM
jrtc27 updated this revision to Diff 223871.Oct 8 2019, 8:22 AM

Rebased, added explicit no-warning test, clang-format'ed, updated assertion message.

jrtc27 marked an inline comment as done.Oct 8 2019, 8:24 AM

I like the patch and I think it is fine.

Small nits:
Could we have a test for " we can detect dodgy pthread_create declarations" and maybe pthread[_attr]_t?
There is an unresolved comment by @shrines.
@probinson: "it seems straightforward enough although clearly needs clang-format-diff run over it."

I'll accept this assuming the above points are easy to fix and given that no one expressed concerns but only positive comments were made.

I believe I have addressed everything, with the possible exception of:

Could we have a test for " we can detect dodgy pthread_create declarations" and maybe pthread[_attr]_t?

Is that not what I had already added to clang/test/Sema/implicit-builtin-decl.c?

rsmith added inline comments.Sep 17 2020, 1:53 PM
clang/include/clang/Serialization/ASTBitCodes.h
1217

This presumably needs to be increased. Are we missing test coverage that would have caught this?

clang/lib/AST/ASTContext.cpp
9234–9237

Our convention for such helper functions is to pass the reference to the associated class (ASTContext in this case) as the first parameter. (I appreciate that DecodeTypeFromStr violates this convention.)

9495–9497

OK. We may eventually want a way to specify noreturn and nothrow here, because they're both part of the type in at least some cases, but this seems fine for the immediate future.

9560

Maybe "missing return type in builtin function type" or similar would be a more useful assert message here?

9577

Please assert(!RequiresICE || !IsNested) here; we shouldn't require parameters of function pointer parameters to be ICEs.

9586–9588

Should we do function -> pointer decay here too, so you can use (...) instead of (...)* in Builtins.def?

9596–9597

Do we need this special case and its associated flag? Can we make GetBuiltinType directly return QualType();for _GetExceptionInfo instead?

9599–9600

This will assert on (.).

clang/lib/Sema/SemaDecl.cpp
1946–1947

Could we use GE_Missing_type here instead? The LIBBUILTIN already lists "pthread.h" as the corresponding header, so that seems sufficient. I think we only need the special cases above to handle the non-LIBBUILTIN cases such as __builtin_fprintf that nonetheless require a header inclusion (in that case to provide FILE).

MaskRay added inline comments.
clang/include/clang/AST/ASTContext.h
2048

You can append a comma so that next time the list gets appended the diff does not have to touch this line.