Page MenuHomePhabricator

Integrated assembler. :vararg macro.
ClosedPublic

Authored by dyatkovskiy on Apr 11 2014, 5:18 AM.

Details

Summary

Please review fix for PR18919, that adds support for :vararg macro in integrated assembler.

Diff Detail

Event Timeline

rnk added a comment.Apr 18 2014, 1:33 PM

IIRC David was hacking on some gnu assembly parsing stuff.

I'd like to see more tests here. .macro statements with normal arguments preceding the :vararg one. Something like:

.macro foo bar, baz:vararg
  ...
.endmacro

A test without a comma between bar and baz would be good as well.

lib/MC/MCParser/AsmParser.cpp
1828–1829

This comment is a bit hard to read, perhaps something more like:

// We want the quotes around the string's contents when parsing for varargs.
3295

Please use /*Varag=*/false to make it clear what is going on.

dyatkovskiy updated this revision to Unknown Object (????).Apr 20 2014, 11:03 PM

Thank you guys!
Everything has been fixed according to your remarks:

  • Fixed comments
  • Added new tests
  • I have also found small bug (thank you for test proposals again) and fixed it too.
dyatkovskiy added inline comments.Apr 20 2014, 11:08 PM
test/MC/AsmParser/vararg.s
20

Sorry for extra lines. Of course they will be removed!

You should probably have a test with a darwin triple too. I cced Jim Grosbach for him to comment if this is a desirable feature for darwin or not. I know the macro system is a bit different, but I don't know the details.

Hi Rafael,
I have implementation for darwin, though in draft state.
There are few reasons/questions:

  1. I didn't find anything about vararg in darwin assembler reference. Or even some .s examples.
  2. If I remember properly, you can substitude parameters by their number: $0, $1, etc. So, may he then, you can refer to some of vararg params as well? Am I right here?
  3. Can I use whole varargs set substitution? Like in Linux tests?
  4. If answer to #2 and #3 is "yes", then I suspect we have to keep vararg values in two forms: as parsed and separated values, and as single string. We need second form, because we need to remember separators as well, in another words we have to expand it as-is. At least we have to keep separators for second form.

Anyway I'm ready to work on it too. But darwin support could contain a bit more changes. Perhaps we could commit it separately?

dyatkovskiy updated this revision to Unknown Object (????).Apr 21 2014, 11:38 PM

Please review updated patch.

  • Added test, that checks :vararg state for darwin.

Thanks!

rafael accepted this revision.Apr 22 2014, 7:17 AM

LGTM with nits.

test/MC/AsmParser/macros-darwin-vararg.s
7

cc is not needed in this test.

test/MC/AsmParser/vararg-default-value.s
17

cc is never 0 in this test, do you need it?

dyatkovskiy closed this revision.Apr 23 2014, 11:12 PM

r206951
Thanks for reviews!