Page MenuHomePhabricator

[Sema] Introduce BuiltinAttr, per-declaration builtin-ness
Needs ReviewPublic

Authored by tambre on Apr 5 2020, 5:08 AM.

Details

Summary

Instead of relying on whether a certain identifier is a builtin, introduce BuiltinAttr to specify a declaration as having builtin semantics.

This fixes incompatible redeclarations of builtins, as reverting the identifier as being builtin due to one incompatible redeclaration would have broken rest of the builtin calls.
Mostly-compatible redeclarations of builtins also no longer have builtin semantics. They don't call the builtin nor inherit their attributes.

Fixes PR45410.

Diff Detail

Unit TestsFailed

TimeTest
320 msClang.Analysis::bstring.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/lib/clang/11.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -verify -analyzer-config eagerly-assume=false /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/clang/test/Analysis/bstring.cpp
520 msClang.Analysis::bstring.cpp
Script: -- : 'RUN: at line 1'; c:\ws\prod\llvm-project\build\bin\clang.exe -cc1 -internal-isystem c:\ws\prod\llvm-project\build\lib\clang\11.0.0\include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -verify -analyzer-config eagerly-assume=false C:\ws\prod\llvm-project\clang\test\Analysis\bstring.cpp
100 msClang.CodeGen::callback_pthread_create.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/lib/clang/11.0.0/include -nostdsysteminc -O1 /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/clang/test/CodeGen/callback_pthread_create.c -S -emit-llvm -o - | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/clang/test/CodeGen/callback_pthread_create.c
130 msClang.CodeGen::callback_pthread_create.c
Script: -- : 'RUN: at line 1'; c:\ws\prod\llvm-project\build\bin\clang.exe -cc1 -internal-isystem c:\ws\prod\llvm-project\build\lib\clang\11.0.0\include -nostdsysteminc -O1 C:\ws\prod\llvm-project\clang\test\CodeGen\callback_pthread_create.c -S -emit-llvm -o - | c:\ws\prod\llvm-project\build\bin\filecheck.exe C:\ws\prod\llvm-project\clang\test\CodeGen\callback_pthread_create.c
370 msClang.CodeGen::ms-intrinsics.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/lib/clang/11.0.0/include -nostdsysteminc -ffreestanding -fms-extensions -fms-compatibility -fms-compatibility-version=17.00 -triple i686--windows -Oz -emit-llvm /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/clang/test/CodeGen/ms-intrinsics.c -o - | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/clang/test/CodeGen/ms-intrinsics.c -check-prefixes CHECK,CHECK-I386,CHECK-INTEL
View Full Test Results (6 Failed)

Event Timeline

tambre created this revision.Apr 5 2020, 5:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2020, 5:08 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
yutsumi accepted this revision.Apr 7 2020, 1:08 AM

Thank you very much. LGTM.

This revision is now accepted and ready to land.Apr 7 2020, 1:08 AM
tambre added a comment.Apr 9 2020, 1:51 AM

rsmith: ping

rsmith: ping 2

Could someone please review this?

hokein added a subscriber: rjmccall.Thu, May 7, 7:42 AM

sorry for the long delay, unfortunately I'm not familiar enough with the code to do a good review.

+cc @rjmccall may have more context (from the git log), could you take a look?

hokein edited reviewers, added: rjmccall; removed: hokein.Thu, May 7, 7:43 AM
hokein added a subscriber: hokein.
rjmccall requested changes to this revision.Thu, May 7, 12:29 PM
rjmccall added inline comments.
clang/lib/Sema/SemaDecl.cpp
3772

The old code didn't check this eagerly. The Old->isImplicit() check is unlikely to fire; please make sure that we don't do any extra work unless it does.

3776

hasRevertedBuiltin() is global state for the identifier, so this check isn't specifically checking anything about the previous declaration, it's checking whether the identifier was ever used for a builtin. Now, the implicit-ness of the previous declaration *is* declaration-specific, and there are very few ways that we end up with an implicit function declaration. It's quite possible that implicit + identifier-is-or-was-a-builtin is sufficient to say that it's an implicit builtin declaration today. However, I do worry about the fact that this change is losing the is-predefined-library-function part of the existing check, and I think we should continue to check that somehow. If that means changing how we revert builtin-ness, e.g. by continuing to store the old builtinID but just recording that it's been reverted, that seems reasonable.

That said, I think there's a larger problem, which is that you're ignoring half of the point of the comment you've deleted:

Doing this for local extern declarations is problematic. If
the builtin declaration remains visible, a second invalid
local declaration will produce a hard error; if it doesn't
remain visible, a single bogus local redeclaration (which is
// actually only a warning) could break all the downstream code.

The problem with reverting the builtin-ness of the identifier after seeing a bad local declaration is that, after we leave the scope of that function, the global function doesn't go back to being a builtin, which can break the behavior of all the rest of the code.

If we want to handle this kind of bogus local declaration correctly, I think we need to stop relying primarily on the builtin-ness of the identifier — which is global state and therefore inherently problematic — and instead start recording whether a specific declaration has builtin semantics. The most obvious way to do that is to record an implicit BuiltinAttr when implicitly declaring a builtin, inherit that attribute only on compatible redeclarations, and then make builtin queries look for that attribute instead of checking the identifier.

Richard, do you agree?

This revision now requires changes to proceed.Thu, May 7, 12:29 PM
tambre updated this revision to Diff 264441.Sat, May 16, 8:29 AM

Rework builtin declaration handling. Introduce BuiltinAttr.

tambre retitled this revision from [Sema] Fix incompatible builtin redeclarations in non-global scope to [Sema] Introduce BuiltinAttr, per-declaration builtin-ness.Sat, May 16, 8:34 AM
tambre edited the summary of this revision. (Show Details)
tambre updated this revision to Diff 264447.Sat, May 16, 10:37 AM
tambre marked 3 inline comments as done.

Fix adding BuiltinAttr in C++ mode. Update one test.

@rsmith This is a big enough architectural change that I'd appreciate your sign-off on the basic approach.

clang/lib/Sema/SemaDecl.cpp
3772

Since BuiltinAttr is inherited, we should keep the isImplicit check, because we only want to use a special diagnostic when "redeclaring" the implicit builtin declaration.

8903

Hmm. I'm concerned about not doing any sort of semantic compatibility check here before we assign the function special semantics. Have we really never done those in C++?

If not, I wonder if we can reasonably make an implicit declaration but just make it hidden from lookup.

tambre updated this revision to Diff 264497.Sun, May 17, 8:14 AM
tambre marked an inline comment as not done.

Semantic compatibility checking for C++ builtin redeclarations.
Remove some now unnecessary logic from getBuiltinID().
Update more tests. 4 tests still failing.

tambre edited the summary of this revision. (Show Details)Sun, May 17, 8:17 AM
tambre marked 4 inline comments as done.EditedSun, May 17, 8:48 AM

Thanks for the reviews and design pointers, John!

There are still a few tests failing, but I'd rather not dive in before the general approach is considered acceptable.

clang/lib/Sema/SemaDecl.cpp
8903

Currently there's no semantic compatibility checking for builtin redeclarations. There is for initial declarations though.

I've added this checking by splitting the actual builtin declaration creation off from LazilyCreateBuiltin into CreateBuiltin and checking if the current declaration is compatible with what the builtin's would be.
This results in stronger typechecking than before for builtin declarations, so some incompatible declarations are no longer marked as builtin. See cxx1z-noexcept-function-type.cpp for an example.

clang/lib/Sema/SemaExpr.cpp
6054

rewriteBuiltinFunctionDecl() creates a new function declaration and happened to disregard the builtin's attributes. This caused BuiltinAttr to go missing in a bunch of OpenCL tests, resulting in failures.

clang/test/Sema/warn-fortify-source.c
13–14

Not quite sure what to do here. These were previously recognized as builtins due to their name despite being incompatible and thus had fortify checking similar to the real memcpy.

Maybe:

  1. Introduce a generic version of ArmBuiltinAliasAttr.
  2. Something like FormatAttr.
clang/test/SemaCXX/cxx11-compat.cpp
35

For some reason this printf didn't get format checking before. Same for SemaCXX/warn-unused-local-typedef.cpp.

clang/test/SemaCXX/cxx1z-noexcept-function-type.cpp
120 ↗(On Diff #264497)

This doesn't work anymore since we now properly check builtin declaration compatibility and since C++17 noexcept is part of the function type but builtins aren't noexcept.
Thoughts?

rjmccall added inline comments.Fri, May 22, 8:40 PM
clang/lib/Sema/SemaDecl.cpp
8903

That makes sense to me in principle. I'm definitely concerned about noexcept differences causing C library functions to not be treated as builtins, though; that seems stricter than we want. How reasonable is it to weaken this?

clang/test/Sema/warn-fortify-source.c
13–14

That's interesting. It definitely seems wrong to apply builtin logic to a function that doesn't have a compatible low-level signature. My inclination is to disable builtin checking here, but to notify the contributors so that they can figure out an appropriate response.

tambre updated this revision to Diff 265871.Sat, May 23, 9:36 AM
tambre marked 6 inline comments as done.

Weakened noexcept checking.

tambre updated this revision to Diff 265872.Sat, May 23, 9:57 AM
tambre marked 4 inline comments as done.

Remove memcpy overload tests from warn-fortify-source.c, which relied on identifier-based builtin identification.

tambre added inline comments.Sat, May 23, 9:58 AM
clang/lib/Sema/SemaDecl.cpp
8903

I agree having noexcept weakened is reasonable.
I've changed it to create an identical type to the NewFD with the exception spec removed for the comparison. This fixes it.

clang/test/CodeGen/ms-intrinsics.c
23 ↗(On Diff #264497)

__stosb and friends aren't marked as builtin because they're declared as static.
I don't think there's a good reason to have builtins as static and we should simply remove the static specifier from those intrinsics in headers.
Alternatively, we could weaken compatibility checking similar to noexcept.
Thoughts?

clang/test/Sema/warn-fortify-source.c
13–14

Agreed.
I've removed this test, as there doesn't seem to be an easy way to replicate this behaviour.

clang/test/SemaCXX/cxx1z-noexcept-function-type.cpp
120 ↗(On Diff #264497)

Fixed by removing noexcept for the declaration compatibility comparison.

aaron.ballman added inline comments.Mon, May 25, 10:42 AM
clang/lib/AST/Decl.cpp
3168–3170

I think this is a bit more clear:

} else if (const auto *A = getAttr<BuiltinAttr>()) {
  BuiltinID = A->getID();
}

and initialize BuiltinID to zero above.

clang/test/Sema/implicit-builtin-decl.c
64

It looks like we're losing test coverage with this change?

tambre updated this revision to Diff 267450.Sat, May 30, 3:20 AM
tambre marked 4 inline comments as done.

Fix some intrinsics not being marked as builtin due to being static in the headers.
Make some code easier to read, fix test for sigsetjmp in Sema/implicit-builtin-decl.c to reflect the original intent.

tambre marked an inline comment as done.Sat, May 30, 3:27 AM
tambre added inline comments.
clang/lib/AST/Decl.cpp
3168–3170

Done.

clang/test/CodeGen/callback_pthread_create.c
17

As many others prior to this patch, pthread_create was recognized as a builtin due to its name and thus had attributes applied.
Unlike others however, pthread_create is the only builtin in Builtins.def that doesn't have its arguments specified. Doing that would require implementing support for function pointers in the builtin database and adding yet another special case for pthread_t and pthread_attr_t.
That'd be quite a bit of work, which I'm not interested in doing.

How about simply removing the hack that is the pthread_create builtin entry and disabling/removing this test?

clang/test/Sema/implicit-builtin-decl.c
64

Indeed. I've reverted this change and changed the test to instead not test for it being recognized as a builtin, since it shouldn't be.