[PATCH] D79991: [TableGen] Fix non-standard escape warnings for braces in InstAlias

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes added inline comments.


================
Comment at: llvm/utils/TableGen/AsmWriterEmitter.cpp:808
           CodeGenInstruction::FlattenAsmStringVariants(CGA.AsmString, Variant);
+      UnescapeString(FlatAliasAsmString);
 
----------------
hfinkel wrote:
> c-rhodes wrote:
> > hfinkel wrote:
> > > Is this unescaping the right thing to do in general, or do we really just want to do it for the braces? What if there were an escaped quote in the string?
> > We probably only want to do this for braces. I wasn't too sure about adding this `UnescapeString` incase we get any undesired behaviour but the tests didn't raise any issues. Maybe it would be better if I create a new function `UnescapeAliasString` to handle this particular case?
> I recommend adding some test cases with some other escapes (quotes, new lines) and, if they don't work correctly, add a `UnescapeAliasString`. If everything does work correctly, then leave it as-is + the extra tests.
I tried some other escapes but don't really know what to make of it as the current behaviour doesn't look right. 

I defined an alias containing an escaped quote as you suggested:

`def FooQuote : InstAlias<"foo \"$imm\"", (foo IntOperand:$imm)>;`

and generated the assembly writer with:

`$ ./bin/llvm-tblgen -gen-asm-writer -I ../llvm/include ../llvm/test/TableGen/AliasAsmString.td -o AsmStringWriter.inc`

the asm string that gets emitted is invalid c++:

```  static const char AsmStrings[] =
    /* 0 */ "foo "$\x01"\0"
  ;```

but the behaviour is the same without this patch. I'm tempted to add `UnescapeAliasString` even though this patch isn't changing escapes in the alias string because I don't know what the correct behaviour is and I don't want to introduce tests that validate potentially incorrect behaviour. Any thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79991