[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:
> 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.
```
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153722