[PATCH] D64834: [Xtensa 8/10] Add support of the Xtensa shift/load/store/move and processor control instructions.

[Andrei Safronov via Phabricator via llvm-commits]

andreisfr added inline comments.


================
Comment at: llvm/lib/Target/Xtensa/AsmParser/XtensaAsmParser.cpp:407
+      return MatchOperand_NoMatch;
+    RegName = StringRef(std::to_string(getLexer().getTok().getIntVal()));
+    break;
----------------
barannikov88 wrote:
> andreisfr wrote:
> > barannikov88 wrote:
> > > andreisfr wrote:
> > > > barannikov88 wrote:
> > > > > Bug: `std::string` termporary is destroyed at ';' and RegName references freed memory.
> > > > Thank you very much, fixed this issue. 
> > > The issue is still there, RegName references deleted memory. One of the possible way to resolve this issue is to declare RegName as std::string rather than StringRef.
> > > 
> > > BTW, what does this code do? I couldn't find any registers that contain only digits in their names (all start with a letter), so MatchRegisterName is bound to fail. Nor could I find a test that uses parenthesized numbers.
> > > 
> > > Are those some special registers not modeled in MC layer (and represented as numbers in assembly language)?
> > > If so, you need to differentiate them from ordinary registers somehow because there is no guarantee that their numbers and / or encodings do not collide with the autogenerated ones for the registers that are being modeled.
> > > 
> > > As a side node, 'SR' variable should have a more descriptive name (or, at least, its name should be explained in a comment).
> > Thank you for comment. The "RegName" variable is not used outside switch cases, so StringRef object should be alive at points where "RegName" actually used,  is it correct? You noted correctly that we should have special registers defined by numbers and Xtensa architecture have such registers for example https://github.com/llvm/llvm-project/blob/ff25800d4ba0b577a44dc918da7a1fb3c29fdb13/llvm/lib/Target/Xtensa/XtensaRegisterInfo.td#L76 . But test for such kind of register names is absent and will be implemented in nearest patches and we will add more explanations to this function.
> RegName is alive, but the data it references is not.
> StringRef does not own the data.
> The data is owned by std::string, which is destroyed before the first use of RegName.
I will rewrite this code  in next patches


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64834