[PATCH] D102707: Fix non-global-value-max-name-size not considered by LLParser

[Hasyimi Bahrudin via Phabricator via llvm-commits]

hasyimibhar added inline comments.


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:3122
+  // lookup will fail if Name exceeds the size cap.
+  Value *Val = F.getValueSymbolTable()->lookup(capLocalValueName(Name));
 
----------------
serge-sans-paille wrote:
> hasyimibhar wrote:
> > serge-sans-paille wrote:
> > > This is starting to looks like an invariant that should be maintained (or at least checked) by the value symbol table itself, instead of being enforced at callsite. 
> > I'm not that familiar with the value symbol table. Does it only contain local value names? If yes, then we can just move the check into the lookup function. If it can also contain global value names, then I'm not sure how it can determine whether or not to cap it just from the provided StringRef.
> It only holds local and args value, so it's fine to apply the check / change in the class itself. But it probably means we need to write a type adapter as the base class is more generic than what we want to implement here. @mehdi_amini does that looks like the right approach to you?
> It only holds local and args value, so it's fine to apply the check / change in the class itself.

I moved it into ValueSymbolTable::lookup.

> But it probably means we need to write a type adapter as the base class is more generic than what we want to implement here. 

I'm not sure what you mean by this. Are you referring to ValueSymbolTable or LLParser?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102707