Inline asm 0bH conflict

[avt77]

Summary

There is a conflict in imm operands between 0b which states "binary" and H which states "Hexa":

int main(){

__asm{
    add al,0bH
}

}

See PR31007 for details

[spatel]

I've never looked at asm parsing. Adding @craig.topper as possible reviewer.

[craig.topper]

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

147

Can you add an explanatory comment here?

lib/MC/MCParser/AsmParser.cpp

740

What is this change for?

[avt77]

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

147

In fact we need here the first line only that tells we're dealing with MS inline asm syntax. The second line is deafult and I can safely remove the one.

lib/MC/MCParser/AsmParser.cpp

740

It's used below in "parseStatement". Without it"parseStatement" crashes.

[RKSimon]

lib/MC/MCParser/AsmParser.cpp

740

Should we even allow a ParseStatementInfo default constructor in that case?

1654

Please explain why we're using the target parser here and not anywhere else that ParsingInlineAsm is used.

2063

clang-format

test/MC/X86/inline-0bh.ll

4 ↗

(On Diff #95555)

please move the triple into the RUN cmd:

llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s

[avt77]

lib/MC/MCParser/AsmParser.cpp

740

From my point of view we should not.

1654

We use "getTargetParser()" below as well. From my point of view we should change the name here. It should be called something like isParsingMSAsm() because this flag really means exactly that.

[avt77]

I added required comments and one additional tiny fix to cover PR27884: now it works properly. The corresponding regression test was added as well.

[ygao]

test/MC/X86/pr27884.s

2

I am not sure whether this test case is supposed to compile cleanly on Linux. Did you test with gnu-as or the latest Intel assembler?
If someone wants to use binary number on Linux and starts their number with "0b" without the "h" suffix, does it still work with your fix?

[RKSimon]

test/MC/X86/inline-0bh.ll

4 ↗

(On Diff #95555)

Also, shouldn't this test file be in test/Codegen/X86 ?

[avt77]

test/MC/X86/pr27884.s

2

Yes, it still works but we should follow gas name conventions:

4 ???? 48031C25      add 0B10101,    %rbx
4      15000000
5 ???? 48031C25      add 0b11100111, %rbx
5      E7000000

[RKSimon]

lib/MC/MCParser/AsmParser.cpp

740

OK - please can you remove the default constructor as part of this patch.

[avt77]

I moved inline-0bh.ll test in test/Codegen/X86 folder.

[RKSimon]

test/CodeGen/X86/inline-0bh.ll

6

rename test function PR31007 to show its origin?

[avt77]

Test function "foo" was renamed as "PR31007" to show its origin.

[avt77]

I inhibitted the default constructor for ParseStatementInfo::ParseStatementInfo() because it can't work without proper initialization.

[RKSimon]

LGTM

[RKSimon]

rL301390

[RKSimon]

Tests added at rL301529

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

void AsmPrinter::EmitInlineAsm(StringRef Str, const MCSubtargetInfo &STI,
   std::unique_ptr<MCInstrInfo> MII(TM.getTarget().createMCInstrInfo());
   std::unique_ptr<MCTargetAsmParser> TAP(TM.getTarget().createMCAsmParser(
       STI, *Parser, *MII, MCOptions));
   if (!TAP)
     report_fatal_error("Inline asm not supported by this streamer because"
                        " we don't have an asm parser for this target\n");
   Parser->setAssemblerDialect(Dialect);
   Parser->setTargetParser(*TAP.get());
+  if (Dialect == InlineAsm::AD_Intel)
+    // We need this flag to be able to parse numbers like "0bH"
+    Parser->setParsingInlineAsm(true);
   if (MF) {
     const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
     TAP->SetFrameRegister(TRI->getFrameRegister(*MF));
   }
 
   emitInlineAsmStart();
   // Don't implicitly switch to the text section before the asm.
   int Res = Parser->Run(/*NoInitialTextSection*/ true,

lib/MC/MCParser/AsmParser.cpp

struct ParseStatementInfo {
   /// \brief The opcode from the last parsed instruction.
   unsigned Opcode = ~0U;
 
   /// \brief Was there an error parsing the inline assembly?
   bool ParseError = false;
 
   SmallVectorImpl<AsmRewrite> *AsmRewrites = nullptr;
 
-  ParseStatementInfo() = default;
+  ParseStatementInfo() = delete;
   ParseStatementInfo(SmallVectorImpl<AsmRewrite> *rewrites)
     : AsmRewrites(rewrites) {}
 };
 
 /// \brief The concrete assembly parser instance.
 class AsmParser : public MCAsmParser {
 private:
   AsmLexer Lexer;
bool AsmParser::Run(bool NoInitialTextSection, bool NoFinalize) {
   if (!NoInitialTextSection)
     Out.InitSections(false);
 
   // Prime the lexer.
   Lex();
 
   HadError = false;
   AsmCond StartingCondState = TheCondState;
+  SmallVector<AsmRewrite, 4> AsmStrRewrites;
 
   // If we are generating dwarf for assembly source files save the initial text
   // section and generate a .file directive.
   if (getContext().getGenDwarfForAssembly()) {
     MCSection *Sec = getStreamer().getCurrentSectionOnly();
     if (!Sec->getBeginSymbol()) {
       MCSymbol *SectionStartSym = getContext().createTempSymbol();
       getStreamer().EmitLabel(SectionStartSym);
       Sec->setBeginSymbol(SectionStartSym);
     }
     bool InsertResult = getContext().addGenDwarfSection(Sec);
     assert(InsertResult && ".text section should not have debug info yet");
     (void)InsertResult;
     getContext().setGenDwarfFileNumber(getStreamer().EmitDwarfFileDirective(
         0, StringRef(), getContext().getMainFileName()));
   }
 
   // While we have input, parse each statement.
   while (Lexer.isNot(AsmToken::Eof)) {
-    ParseStatementInfo Info;
+    ParseStatementInfo Info(&AsmStrRewrites);
     if (!parseStatement(Info, nullptr))
       continue;
 
     // If we have a Lexer Error we are on an Error Token. Load in Lexer Error
     // for printing ErrMsg via Lex() only if no (presumably better) parser error
     // exists.
     if (!hasPendingError() && Lexer.getTok().is(AsmToken::Error)) {
       Lex();
case AsmToken::Colon: {
 
     // Consume any end of statement token, if present, to avoid spurious
     // AddBlankLine calls().
     if (getTok().is(AsmToken::EndOfStatement)) {
       Lex();
     }
 
     // Emit the label.
-    if (!ParsingInlineAsm)
+    if (!getTargetParser().isParsingInlineAsm())
       Out.EmitLabel(Sym, IDLoc);
 
     // If we are generating dwarf for assembly source files then gather the
     // info to make a dwarf label entry for this label if needed.
     if (getContext().getGenDwarfForAssembly())
       MCGenDwarfLabelEntry::Make(Sym, &getStreamer(), getSourceManager(),
                                  IDLoc);
 
getStreamer().EmitDwarfLocDirective(
         getContext().getGenDwarfFileNumber(), Line, 0,
         DWARF2_LINE_DEFAULT_IS_STMT ? DWARF2_FLAG_IS_STMT : 0, 0, 0,
         StringRef());
   }
 
   // If parsing succeeded, match the instruction.
   if (!ParseHadError) {
     uint64_t ErrorInfo;
-    if (getTargetParser().MatchAndEmitInstruction(IDLoc, Info.Opcode,
-                                                  Info.ParsedOperands, Out,
-                                                  ErrorInfo, ParsingInlineAsm))
+    if (getTargetParser().MatchAndEmitInstruction(
+            IDLoc, Info.Opcode, Info.ParsedOperands, Out, ErrorInfo,
+            getTargetParser().isParsingInlineAsm()))
       return true;
   }
   return false;
 }
 
 // Parse and erase curly braces marking block start/end
 bool 
 AsmParser::parseCurlyBlockScope(SmallVectorImpl<AsmRewrite> &AsmStrRewrites) {

lib/Target/X86/AsmParser/X86AsmParser.cpp

 bool X86AsmParser::ParseDirective(AsmToken DirectiveID) {
   MCAsmParser &Parser = getParser();
   StringRef IDVal = DirectiveID.getIdentifier();
   if (IDVal == ".word")
     return ParseDirectiveWord(2, DirectiveID.getLoc());
   else if (IDVal.startswith(".code"))
     return ParseDirectiveCode(IDVal, DirectiveID.getLoc());
   else if (IDVal.startswith(".att_syntax")) {
+    getParser().setParsingInlineAsm(false);
     if (getLexer().isNot(AsmToken::EndOfStatement)) {
       if (Parser.getTok().getString() == "prefix")
         Parser.Lex();
       else if (Parser.getTok().getString() == "noprefix")
         return Error(DirectiveID.getLoc(), "'.att_syntax noprefix' is not "
                                            "supported: registers must have a "
                                            "'%' prefix in .att_syntax");
     }
     getParser().setAssemblerDialect(0);
     return false;
   } else if (IDVal.startswith(".intel_syntax")) {
     getParser().setAssemblerDialect(1);
+    getParser().setParsingInlineAsm(true);
     if (getLexer().isNot(AsmToken::EndOfStatement)) {
       if (Parser.getTok().getString() == "noprefix")
         Parser.Lex();
       else if (Parser.getTok().getString() == "prefix")
         return Error(DirectiveID.getLoc(), "'.intel_syntax prefix' is not "
                                            "supported: registers must not have "
                                            "a '%' prefix in .intel_syntax");
     }

test/CodeGen/X86/inline-0bh.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
+
+; Function Attrs: noinline nounwind
+define i32 @PR31007() {
+; CHECK-LABEL: PR31007:
+; CHECK:       # BB#0: # %entry
+; CHECK-NEXT:  #APP
+; CHECK   :    addb $11, %al
+; CHECK:       #NO_APP
+; CHECK-NEXT:  xorl %eax, %eax
+; CHECK-NEXT:  retq
+entry:
+  call void asm sideeffect inteldialect "add al,$$0bH", "~{al},~{flags},~{dirflag},~{fpsr},~{flags}"()
+  ret i32 0
+}
+

test/MC/X86/pr27884.s

+// RUN: llvm-mc -triple x86_64-unknown-unknown %s
+
+.intel_syntax
+add rbx, 0B0h
+add rbx, 0b0h
+add rax, 0A0h
+add rax, 0a0h