Skip to content

[clang] Fix a dangling reference in clang/utils/TableGen/ClangDiagnosticsEmitter.cpp #119197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 10, 2024
Merged

[clang] Fix a dangling reference in clang/utils/TableGen/ClangDiagnosticsEmitter.cpp #119197

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85f1d54
[clang] Fix a dangling reference in clang/utils/TableGen/ClangDiagnos…
hokein Dec 9, 2024
294ce92
clang-format
hokein Dec 9, 2024
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
3 changes: 1 addition & 2 deletions clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1907,8 +1907,7 @@ void clang::EmitClangDiagDocs(const RecordKeeper &Records, raw_ostream &OS) {
// Write out the diagnostic groups.
for (const Record *G : DiagGroups) {
bool IsRemarkGroup = isRemarkGroup(G, DiagsInGroup);
auto &GroupInfo =
DiagsInGroup[std::string(G->getValueAsString("GroupName"))];
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe replace with llvm::StringMap to get better performance and avoid these problems for good?
I would be surprised if the StringRef in the map is critical for performance and I am 99% certain that StringMap will likely win over std::map<StringRef, ... anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting, there is a recent commit which changes the map<std::string, ...> to map<llvm::StringRef, ...>, 17c6ec6 , which leads to this dangling reference, cc @jurahul

If we want to change to StringMap, I think it is a separate change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, changing to StringMap would be a different change. Also, we need to check if that ok to do in case we iterate over the map in a way that requires deterministic iteration order (and if StringMap does not guarantee that).

Can you fix the clang-format issue before approving?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you fix the clang-format issue before approving?

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we need to have stable order, let's document it. Could you elaborate what is "different" about a change to StringMap?

It's a container that does key-value lookups with string keys. It is efficient, widely used in LLVM and fully avoids this class of errors. Are there any downsides of using it (assuming we don't need to stable iteration order that std::map provides?)

Copy link
Contributor

@jurahul jurahul Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, we need to document it. I can see from the code that we iterate over the map (in emitDiagTable)

  for (auto const &[Name, GroupInfo] : DiagsInGroup) {

with std::map, this iteration happens in an order sorted by the keys in the map. And that is needed so that the tablegen generated code is deterministic. I don't think StringMap has similar iteration order guarantees. See https://llvm.org/docs/ProgrammersManual.html#llvm-adt-stringmap-h

StringMap iteration order, however, is not guaranteed to be deterministic, so any uses which require that should instead use a std::map.

auto &GroupInfo = DiagsInGroup[G->getValueAsString("GroupName")];
bool IsSynonym = GroupInfo.DiagsInGroup.empty() &&
GroupInfo.SubGroups.size() == 1;

Expand Down
Loading