[LLParser] Error out if a name is too long and gets renamed

[aeubanks]

Summary

Typically names longer than -non-global-value-max-name-size will just get renamed if there is a collision after truncating. This is fine since we typically don't reference Values by name.

However LLParser does reference Values by name, so report an error when that happens, otherwise weird issues can crop up if there are name collisions (e.g. verifier issues with the changed test case because we end up reusing the same block for testz and testa).

[aeubanks]

update

[arsenm]

I’d expect the names to be truncated to the limit plus the conflict resolution number. Does the limit clamp the suffix?

[aeubanks]

In D145282#4168678, @arsenm wrote:

I’d expect the names to be truncated to the limit plus the conflict resolution number. Does the limit clamp the suffix?

yes that is how it works, the limit doesn't clamp the suffix, but that's not the issue

the issue is that within LLParser, F.getValueSymbolTable()->lookup() truncates the name, resulting in both testa and testz getting mapped to the same BasicBlock in LLParser::PerFunctionState::getVal

the original issue was https://github.com/llvm/llvm-project/issues/45244, which https://reviews.llvm.org/D102707 sort of fixed. but the issue of collisions was never raised and I ran into it with very long basic block names. I think it's possible to fully fix this issue by keeping track of renaming but it's not trivial (e.g. have to handle the case where we see testz, truncate it to test internally, then see an actual test later on), and a solution wouldn't be free in terms of compile time

[nikic]

LG, I think it's fine to just forbid this.

llvm/lib/AsmParser/LLParser.cpp

Value *LLParser::PerFunctionState::getVal(const std::string &Name, Type *Ty,
 
   // Otherwise, create a new forward reference for this value and remember it.
   Value *FwdVal;
   if (Ty->isLabelTy()) {
     FwdVal = BasicBlock::Create(F.getContext(), Name, &F);
   } else {
     FwdVal = new Argument(Ty, Name);
   }
+  if (FwdVal->getName() != Name) {
+    P.error(Loc, "name is too long which can result in name collisions, "
+                 "consider making the name shorter or "
+                 "increasing -non-global-value-max-name-size");
+    return nullptr;
+  }
 
   ForwardRefVals[Name] = std::make_pair(FwdVal, Loc);
   return FwdVal;
 }
 
 Value *LLParser::PerFunctionState::getVal(unsigned ID, Type *Ty, LocTy Loc) {
   // Look this name up in the normal function symbol table.
   Value *Val = ID < NumberedVals.size() ? NumberedVals[ID] : nullptr;

llvm/test/Assembler/non-global-value-max-name-size.ll

-; RUN: opt < %s -S -non-global-value-max-name-size=4
-; Test that local value name lookup works if the name is capped
+; RUN: opt < %s -S -non-global-value-max-name-size=5
+; RUN: not opt < %s -S -non-global-value-max-name-size=4 2>&1 | FileCheck %s
+
+; CHECK: name is too long
 
 define void @f() {
 bb0:
   br label %testz
 
 testz:
+  br label %testa
+
+testa:
   br label %testz
 }