Fix to attribute plugins reaching an unreachable

[amykhuang]

0faee97 broke attribute plugins. Specifically, it added a call to getAttributeSpellingListIndex() in situations that reached an unreachable statement. This patch adds a check before calling that to avoid hitting the unreachable.

clang/test/Frontend/plugin-attribute.cpp has been broken since 0faee97, and this patch fixes it.

Bug: 70702

[llvmbot]

@llvm/pr-subscribers-clang

Author: Amy Huang (amykhuang)

Changes

0faee97 made the attribute plugin code hit an unreachable.

Bug: 70702

---

Full diff: https://github.com/llvm/llvm-project/pull/70877.diff

1 Files Affected:

• (modified) clang/lib/Sema/ParsedAttr.cpp (+6-2)

diff --git a/clang/lib/Sema/ParsedAttr.cpp b/clang/lib/Sema/ParsedAttr.cpp
index f59b01efe7ed8f4..2d6d17e74f6e38d 100644
--- a/clang/lib/Sema/ParsedAttr.cpp
+++ b/clang/lib/Sema/ParsedAttr.cpp
@@ -193,9 +193,13 @@ bool ParsedAttr::isTypeAttr() const { return getInfo().IsType; }
 bool ParsedAttr::isStmtAttr() const { return getInfo().IsStmt; }
 
 bool ParsedAttr::existsInTarget(const TargetInfo &Target) const {
+  Kind K = getParsedKind();
+  bool HasSpelling = K != IgnoredAttribute && K != UnknownAttribute &&
+                     K != NoSemaHandlerAttribute;
   return getInfo().existsInTarget(Target) &&
-         getInfo().spellingExistsInTarget(Target,
-                                          getAttributeSpellingListIndex());
+         (HasSpelling &&
+          getInfo().spellingExistsInTarget(Target,
+                                          getAttributeSpellingListIndex()));
 }
 
 bool ParsedAttr::isKnownToGCC() const { return getInfo().IsKnownToGCC; }

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[shafik]

Can you please add a more detailed description of the problem and what the fix actually is. The description is what ends up in the git log and it is important that we have enough details there to understand the PR and what changes it makes.

I do not see a test, can this fix be tested?

[AaronBallman]

Can you please add a more detailed description of the problem and what the fix actually is. The description is what ends up in the git log and it is important that we have enough details there to understand the PR and what changes it makes.

I do not see a test, can this fix be tested?

We could add a test with a // REQUIRES: plugins, examples line on the RUN line. We don't have very many tests for plugins in Clang currently, but adding one here would help catch regressions.

[AaronBallman]

As far as the changes go, they look correct to me. Thank you for the fix! Can you try adding test coverage for the change?

[amykhuang]

Huh, apparently my no-unique-address patch did break existing plugin tests (e.g. clang/test/Frontend/plugin-attribute.cpp), and this patch doesn't fix it

[amykhuang]

Updated the PR description and the patch. The existing clang/test/Frontend/plugin-attribute.cpp seems sufficient for test coverage. (I guess it wasn't being tested on bots though, otherwise we would have seen it?)

[amykhuang]

ping for review?

[AaronBallman]

ping for review?

My apologies for the delay!

The changes LGTM but there is a precommit CI failure with libc++. As best I can tell, that looks to be a transient issue and not caused by your patch, but it would be good to verify that before landing. (I would be really surprised if this change managed to break the demangler behavior.) CC @ldionne @mordante @philnik777 for other opinions.

[ldionne]

This is weird, it's the first time I see this and I couldn't find any obvious offending commit. It looks like it shouldn't be related, but at the same time we're not seeing this issue in any of the other builds in the Clang pipeline. You could consider updating the PR with the latest main content to re-trigger a CI run and see if that still fails.

[AaronBallman]

Oh wow, same failure a second time...

Ohh, this looks familiar now, but... it was already fixed, so why are we still hitting this failure?

| expected: clang::TreeTransform<(anonymous namespace)::CurrentInstantiationRebuilder>::RebuildArrayType(clang::QualType, clang::ArraySizeModifier, llvm::APInt const*, clang::Expr*, unsigned int, clang::SourceRange)
| got: 0,   clang::TreeTransform<(anonymous namespace)::CurrentInstantiationRebuilder>::RebuildArrayType(clang::QualType, clang::ArrayType::ArraySizeModifier, llvm::APInt const*, clang::Expr*, unsigned int, clang::SourceRange)

should have been fixed by f5f4c5b over two weeks ago. CC @Endilll

[Endilll]

This was fixed by e11148f
I checked the logs, and exactly the part that this commit fixed is failing.

[Endilll]

https://github.com/amykhuang/llvm-project/tree/attr-plugin
This branch is 2 commits ahead, 1684 commits behind llvm:main.

Yeah, this branch is missing all November commits, including aforementioned fix.

[amykhuang]

Whoops, sorry about that, I guess I am failing to update my fork properly...

[AaronBallman]

LGTM assuming precommit CI is now fixed.