Page MenuHomePhabricator

Inline asm 0bH conflict
ClosedPublic

Authored by avt77 on Apr 18 2017, 5:50 AM.

Details

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

Diff Detail

Event Timeline

avt77 created this revision.Apr 18 2017, 5:50 AM
spatel edited reviewers, added: craig.topper; removed: spatel.Apr 18 2017, 6:49 AM
spatel edited edge metadata.
spatel added a subscriber: craig.topper.

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

craig.topper added inline comments.Apr 18 2017, 11:26 PM
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 added inline comments.Apr 19 2017, 12:38 AM
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 added inline comments.Apr 19 2017, 3:22 AM
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 added inline comments.Apr 19 2017, 4:15 AM
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 updated this revision to Diff 95724.Apr 19 2017, 5:50 AM

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 added a subscriber: ygao.Apr 19 2017, 11:22 AM
ygao added inline comments.
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 added inline comments.Apr 19 2017, 12:16 PM
test/MC/X86/inline-0bh.ll
4 ↗(On Diff #95555)

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

avt77 added inline comments.Apr 20 2017, 1:41 AM
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 added inline comments.Apr 23 2017, 5:32 AM
lib/MC/MCParser/AsmParser.cpp
740

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

avt77 updated this revision to Diff 96361.Apr 24 2017, 2:40 AM

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

RKSimon added inline comments.Apr 24 2017, 4:04 AM
test/CodeGen/X86/inline-0bh.ll
6

rename test function PR31007 to show its origin?

avt77 updated this revision to Diff 96378.Apr 24 2017, 4:54 AM

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

avt77 updated this revision to Diff 96532.Apr 25 2017, 5:17 AM

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

RKSimon accepted this revision.Apr 25 2017, 5:27 AM

LGTM

This revision is now accepted and ready to land.Apr 25 2017, 5:27 AM