[LLParser] Friendly error out if a non global value's name exceeds max name size

[wheatfox]

Summary

The previous implementation of LLParser.cpp will error out
multiple definition of local value when %var = ... exceeds max length(when set to 2). However, the real error is name size exceeding. Therefore, I added some code to correctly error out that reason instead of "multiple definition".

[wheatfox]

update llvm\test\Assembler\non-global-value-max-name-size.ll and add llvm-as test

[wheatfox]

designed a new test in \llvm\test\Assembler\non-global-value-max-name-size-2.ll, and used llvm-lit to run the new test locally to make sure that things work fine.

[nikic]

llvm/lib/AsmParser/LLParser.cpp

3447

So this is based on the assumption that in the "multiple definition" case the name will always be %name0? This seems a bit fragile (I'm not entirely confident it is true, and we can't reach a counter with more than one digit here). Is it possible to check InstNameSize < NameStrSize instead?

[wheatfox]

llvm/lib/AsmParser/LLParser.cpp

3447

according to createValueName and makeUniqueName, if we have a code of

%var = add i32 0, 1
%var = alloca i32, align 4

then the first var will be fine, but the second var will be appended a counter S << ++LastUnique in makeUniqueName, so it will store var1 as the value's name. Then the parser throws the "multiple definition" error immediately, so this suffix 1 will only exist for once(and also for a short time since the error will be thrown at once).

Moreover. I agree that InstNameSize < NameStrSize will be more intuitive for "name size overflowed". If InstNameSize != NameStrSize + 1 and obviously InstNameSize != NameStrSize(considering the first if Inst->getName() != NameStr, if the size are the same, then the two strings should be exactly the same), then InstNameSize < NameStrSize exactly matches the case. I will update my diff later :)

[wheatfox]

llvm/lib/AsmParser/LLParser.cpp

3447

I also found another issue:

define void @f() {
bb0:
  %var.1 = alloca i32, align 4
  %var.11 = alloca i32, align 4
  ret void
}

this code will sucessfully run when max name size is set to 5, even if var.11 has size of 6. This is because var.11 was first trimmed to var.1 due to 5's limit, and then makeUniqueName append 1 to its end. Finally we got var.11 again, which passed the Inst->getName() != NameStr since the actual var.11 equals the new name var.11`. I will consider this along with my patch.

[wheatfox]

llvm/lib/AsmParser/LLParser.cpp

3447

define void @f() {
bb0:
  %varname = alloca i32, align 4
  %varname2 = add i32 0, 1
  ret void
}

According to the new found issue, in the case above (max name size=7) while varname2 was trimmed into varname and conflicts the first varname, the Inst->getName() will be varname1 which will be the same size of NameStr, Therefore, only InstNameSize != NameStrSize + 1 will work.

[wheatfox]

considered another scene where potential name size exceeding would happen

[wheatfox]

cleanup the comments

llvm/lib/AsmParser/LLParser.cpp

if (FI != ForwardRefVals.end()) {
     Sentinel->replaceAllUsesWith(Inst);
     Sentinel->deleteValue();
     ForwardRefVals.erase(FI);
   }
 
   // Set the name on the instruction.
   Inst->setName(NameStr);
 
-  if (Inst->getName() != NameStr)
+  if (Inst->getName() != NameStr) {
+
+    size_t InstNameSize = Inst->getName().size();
+    size_t NameStrSize = NameStr.size();
+
+    if (InstNameSize != NameStrSize + 1) {
+      // The name is too long and was trimmed when stored in SmallString with
+      // NonGlobalValueMaxNameSize
+      return P.error(NameLoc, "name length exceeds non global max name size, "
+                              "consider making the name shorter or "
+                              "increasing -non-global-value-max-name-size: '" +
+                                  NameStr + "'");
+    }
+
     return P.error(NameLoc, "multiple definition of local value named '" +
                                 NameStr + "'");
+  }
   return false;
 }
 
 /// getBB - Get a basic block with the specified name or ID, creating a
 /// forward reference record if needed.
 BasicBlock *LLParser::PerFunctionState::getBB(const std::string &Name,
                                               LocTy Loc) {
   return dyn_cast_or_null<BasicBlock>(

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

 ; RUN: opt < %s -S -non-global-value-max-name-size=5
+; RUN: llvm-as %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:
+  %var.1 = alloca i32, align 4
   br label %testz
 
 testz:
   br label %testa
 
 testa:
   br label %testz
 }