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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/utils/TableGen/AsmWriterEmitter.cpp:808
           CodeGenInstruction::FlattenAsmStringVariants(CGA.AsmString, Variant);
+      UnescapeString(FlatAliasAsmString);
 
----------------
c-rhodes wrote:
> 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?
Fair enough; the non-compilable code certainly isn't useful, but maybe we've never really supported things in the strings that would need escaping when output anyway. If so, this is a special case distinct from those.


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

https://reviews.llvm.org/D79991