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

[Han Yulong via Phabricator via llvm-commits]

wheatfox added inline comments.


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:3447
+
+    if (InstNameSize != NameStrSize + 1) {
+      // The name is too long and was trimmed when stored in SmallString with
----------------
wheatfox wrote:
> nikic wrote:
> > 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?
> 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 :)
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153722/new/

https://reviews.llvm.org/D153722