Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,7 @@ Bug Fixes in This Version
- Fix crash due to unknown references and pointer implementation and handling of
base classes. (GH139452)
- Fixed an assertion failure in serialization of constexpr structs containing unions. (#GH140130)
- Fixed duplicate entries in TableGen that caused the wrong attribute to be selected. (GH#140701)

Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
12 changes: 12 additions & 0 deletions clang/test/AST/ast-dump-riscv-attributes.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// RUN: %clang_cc1 -triple riscv64 -ast-dump -ast-dump-filter c23 -std=c23 -x c %s | FileCheck --strict-whitespace %s

// CHECK: FunctionDecl{{.*}}pre_c23
// CHECK-NEXT: |-CompoundStmt
// CHECK-NEXT: `-RISCVInterruptAttr{{.*}}supervisor
__attribute__((interrupt("supervisor"))) void pre_c23(){}

// CHECK: FunctionDecl{{.*}}in_c23
// CHECK-NEXT: |-CompoundStmt
// CHECK-NEXT: `-RISCVInterruptAttr{{.*}}supervisor
// CHECK-NOT: `-RISCVInterruptAttr{{.*}}machine
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this test. Why does this choose the RISCV interrupt instead of the GNU one? And where would 'machine' come from?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"-triple riscv64" is why you get the RISCVInterruptAttr, which is expected.

"machine" is on a CHECK-NOT line, which is what the bug was about.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the question is 'how'? Is the "supervisor" not coming from the string?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this patch, our "has attribute" check was looking through a StringSwitch that had multiple cases with the same string argument but different resulting values (that would check for target triples and arches). So we'd hit the first case, say "this matches", and the test for the attribute would only pass if the target triple matched. Because the first triple was never the RISCV attribute, we'd always say "we don't have this attribute". In turn, this would mean parsing would say "well, we don't know about this attribute, so eat all the arguments". But because Sema could figure out which handleInterruptAttr function to call, we'd actually go ahead and make the correct attribute, just with the incorrect argument. The default if no argument is specified for RISCV is machine. So we'd ignore supervisor and just substitute in machine quietly for the user.

[[gnu::interrupt("supervisor")]] void in_c23(){}
31 changes: 26 additions & 5 deletions clang/utils/TableGen/ClangAttrEmitter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSwitch.h"
#include "llvm/Support/ErrorHandling.h"
Expand Down Expand Up @@ -3667,6 +3668,12 @@ static bool GenerateTargetSpecificAttrChecks(const Record *R,
static void GenerateHasAttrSpellingStringSwitch(
ArrayRef<std::pair<const Record *, FlattenedSpelling>> Attrs,
raw_ostream &OS, StringRef Variety, StringRef Scope = "") {

// It turns out that there are duplicate records for a given spelling. This
// map combines matching test strings using '||'. For example, if there are
// three conditions A, B, and C, the final result will be: A || B || C.
llvm::StringMap<std::string> TestStringMap;

for (const auto &[Attr, Spelling] : Attrs) {
// C++11-style attributes have specific version information associated with
// them. If the attribute has no scope, the version information must not
Expand Down Expand Up @@ -3727,12 +3734,26 @@ static void GenerateHasAttrSpellingStringSwitch(
}
}

std::string TestStr = !Test.empty()
? Test + " ? " + itostr(Version) + " : 0"
: itostr(Version);
if (Scope.empty() || Scope == Spelling.nameSpace())
OS << " .Case(\"" << Spelling.name() << "\", " << TestStr << ")\n";
std::string TestStr =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you share a before/after generated file so that I can take a look? I'm having a hard time visualizing what this does.

Copy link
Contributor Author

@Mr-Anyone Mr-Anyone May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take the interrupt attribute as an example:

This is the original string: true && (T.getArch() == llvm::Triple::riscv32 || T.getArch() == llvm::Triple::riscv64) ? 1 : 0.

The problem is that there are multiple of them for a single attribute, such as true && (T.getArch() == llvm::Triple::arm || T.getArch() == llvm::Triple::thumb || T.getArch() == llvm::Triple::armeb || T.getArch() == llvm::Triple::thumbeb) ? 1 : 0 for arm.

Meaning that, we used to get:

.Case("interrupt",true && (T.getArch() == llvm::Triple::riscv32 || T.getArch() == llvm::Triple::riscv64) ? 1 : 0 )
.Case("interrupt",true && (T.getArch() == llvm::Triple::arm || T.getArch() == llvm::Triple::thumb || T.getArch() == llvm::Triple::armeb || T.getArch() == llvm::Triple::thumbeb) ? 1 : 0
...

This is the new one TestStr: (true && (T.getArch() == llvm::Triple::avr) ? 1 : 0), with the only difference being the added parentheses. We can join test strings together with ||, meaning we generate something like this:

(true && (T.getArch() == llvm::Triple::riscv32 || T.getArch() == llvm::Triple::riscv64) ? 1 : 0) ||
                                (true && (T.getArch() == llvm::Triple::arm || T.getArch() == llvm::Triple::thumb || T.getArch() == llvm::Triple::armeb || T.getArch() == llvm::Triple::thumbeb) ? 1 : 0) ... 

So now we have a single case statement instead,

.Case("interrupt",(true && (T.getArch() == llvm::Triple::riscv32 || T.getArch() == llvm::Triple::riscv64) ? 1 : 0) ||
                                (true && (T.getArch() == llvm::Triple::arm || T.getArch() == llvm::Triple::thumb || T.getArch() == llvm::Triple::armeb || T.getArch() == llvm::Triple::thumbeb) ? 1 : 0) ... )
...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thank you for that!

!Test.empty() ? '(' + Test + " ? " + itostr(Version) + " : 0" + ')'
: '(' + itostr(Version) + ')';

if (Scope.empty() || Scope == Spelling.nameSpace()) {
if (TestStringMap.contains(Spelling.name())) {
TestStringMap[Spelling.name()] += " || " + TestStr;
} else {
TestStringMap[Spelling.name()] = TestStr;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (TestStringMap.contains(Spelling.name())) {
TestStringMap[Spelling.name()] += " || " + TestStr;
} else {
TestStringMap[Spelling.name()] = TestStr;
}
if (TestStringMap.contains(Spelling.name()))
TestStringMap[Spelling.name()] += " || " + TestStr;
else
TestStringMap[Spelling.name()] = TestStr;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, thanks.

}
}

// Create the actual string switch statement after all the attributes have
// been parsed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// been parsed
// been parsed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

for (auto &entry : TestStringMap) {
OS << " .Case(\"" << entry.getKey() << "\", " << entry.getValue()
<< ")\n";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (auto &entry : TestStringMap) {
OS << " .Case(\"" << entry.getKey() << "\", " << entry.getValue()
<< ")\n";
for (auto &Entry : TestStringMap) {
OS << " .Case(\"" << Entry.getKey() << "\", " << Entry.getValue()
<< ")\n";

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

}

OS << " .Default(0);\n";
}

Expand Down