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

[hokein]

DiagsInGroup is a map<llvm::StringRef, ...>, we store a dangling string_view in the key.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Haojian Wu (hokein)

Changes

DiagsInGroup is a map<llvm::StringRef, ...>, we store a dangling string_view in the key.

---

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

1 Files Affected:

• (modified) clang/utils/TableGen/ClangDiagnosticsEmitter.cpp (+1-1)

diff --git a/clang/utils/TableGen/ClangDiagnosticsEmitter.cpp b/clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
index 6a4a64a0813063..f7a6807a142490 100644
--- a/clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
+++ b/clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
@@ -1908,7 +1908,7 @@ void clang::EmitClangDiagDocs(const RecordKeeper &Records, raw_ostream &OS) {
   for (const Record *G : DiagGroups) {
     bool IsRemarkGroup = isRemarkGroup(G, DiagsInGroup);
     auto &GroupInfo =
-        DiagsInGroup[std::string(G->getValueAsString("GroupName"))];
+        DiagsInGroup[G->getValueAsString("GroupName")];
     bool IsSynonym = GroupInfo.DiagsInGroup.empty() &&
                      GroupInfo.SubGroups.size() == 1;
 

[github-actions]

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

[ilya-biryukov]

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.

[hokein]

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.

[jurahul]

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?

[hokein]

Can you fix the clang-format issue before approving?

Done.

[ilya-biryukov]

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?)

[jurahul]

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.

[hokein]

I'm landing this change now since it is a straightforward use-after-free fix, happy to address any followups.