[PATCH] D77491: [Sema] Fix incompatible builtin redeclarations in non-global scope

[John McCall via Phabricator via cfe-commits]

rjmccall requested changes to this revision.
rjmccall added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:3759
   unsigned BuiltinID;
-  if (Old->isImplicit() && (BuiltinID = Old->getBuiltinID())) {
+  bool RevertedBuiltin{Old->getIdentifier()->hasRevertedBuiltin()};
+  if (Old->isImplicit() &&
----------------
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.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:3765
+    // Also warn if it's a redeclaration of a builtin redeclaration. We know if
+    // it is if the previous declaration is a reverted builtin.
+    if (RevertedBuiltin ||
----------------
`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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77491/new/

https://reviews.llvm.org/D77491