This is an archive of the discontinued LLVM Phabricator instance.

Adding option -fno-inline-asm to disallow inline asm
ClosedPublic

Authored by steven_wu on Jan 7 2015, 2:42 PM.

Details

Summary

This patch add a new option to dis-allow all inline asm.
Any GCC style inline asm will be reported as an error.

Diff Detail

Event Timeline

steven_wu updated this revision to Diff 17874.Jan 7 2015, 2:42 PM
steven_wu retitled this revision from to Adding option -fno-inline-asm to disallow inline asm.
steven_wu updated this object.
steven_wu edited the test plan for this revision. (Show Details)
steven_wu added a subscriber: Unknown Object (MLST).
rsmith added a subscriber: rsmith.Jan 14 2015, 7:41 PM

The flag name doesn't match its behavior here: it disables GNU C inline asm only, not CodeView/MSVC-style inline asm. What's the intent here? Why should we allow this particular GNU extension to be disabled? Would this make more sense as part of a (much-discussed but never implemented) -fno-gnu-extensions flag?

The intention of this patch is to provide a way to control the usage of inline assembly since they are harder to get correct. The reason why it is not issuing an error on MSVC style assembly is because we already have a way to control its usage (-fno-asm-block).
I can imply -fno-asm-block with -fno-inline-asm but that will require some refactoring of my patch since these two flags triggers different error with this patch. -fno-inline-asm is currently a sema check and -fno-asm-block will disable a path in the parser.
It is a good point that we can use this patch to get a one-step closer to -fno-gnu-extensions. In that case, I think it should be better to make a new flag (-fno-gnu-asm) which disable gnu-style asm parser, and -fno-inline-asm should implies both -fno-asm-block and -fno-gnu-asm.
I will try to come up with a new patch tomorrow.

Yes please, the confusing option wasn't really good.

steven_wu updated this revision to Diff 18276.Jan 15 2015, 4:27 PM
  • Another attempt to implement -fno-inline-asm according to the feedback. Now implemented a new flag -fno-gnu-inline-asm which return error when parsing GNU-style inline assembly. -fno-inline-asm will just implies both -fno-gnu-inline-asm and -fno-asm-blocks

Hi Richard and Eric

Let me know what you think about this approach! Thanks for the feedback!

Steven

rnk accepted this revision.Jan 15 2015, 4:40 PM
rnk added a reviewer: rnk.
rnk added a subscriber: rnk.

lgtm

Unlike -fno-ms-extensions, this doesn't make us blind to __asm__ like we are for __fastcall & co. That seems OK.

Maybe this should this also affect asm register variables like this?

int foo1 asm ("bar1");
include/clang/Basic/LangOptions.def
117

Let's invert this to GNUAsm and make it on by default.

This revision is now accepted and ready to land.Jan 15 2015, 4:40 PM
steven_wu updated this revision to Diff 18284.Jan 15 2015, 5:49 PM
steven_wu edited edge metadata.
  • Update patch that AsmLabels are now disabled with -fno-gnu-inline-asm. This also means functions with asm-labels will also be reported as an error with -fno-gnu-inline-asm.
steven_wu updated this revision to Diff 18286.Jan 15 2015, 6:36 PM
  • Fix the code format and fix the comments. Sorry for the spam
rnk added a comment.Jan 16 2015, 10:14 AM
  • Update patch that AsmLabels are now disabled with -fno-gnu-inline-asm. This also means functions with asm-labels will also be reported as an error with -fno-gnu-inline-asm.

I wasn't suggesting that we reject function asm labels, I was suggesting that we reject variables marked as living in specific registers. Consider something like:

register void *sp asm("esp");
int main () {
  printf("sp: %p\n", sp);
}

This code compiles down to read the esp register in main.

I think using asm labels to rename something is not inherently architecture specific and could be accepted even in the presence of -fno-inline-asm. What do you think?

In D6870#109832, @rnk wrote:
  • Update patch that AsmLabels are now disabled with -fno-gnu-inline-asm. This also means functions with asm-labels will also be reported as an error with -fno-gnu-inline-asm.

I wasn't suggesting that we reject function asm labels, I was suggesting that we reject variables marked as living in specific registers. Consider something like:

register void *sp asm("esp");
int main () {
  printf("sp: %p\n", sp);
}

This code compiles down to read the esp register in main.

I think using asm labels to rename something is not inherently architecture specific and could be accepted even in the presence of -fno-inline-asm. What do you think?

I agree that function asm labels are a separate thing and should not be affected by -fno-gnu-inline-asm. It would still be good to disallow register asm with that flag, though.

I like the suggestion to call this option "-fno-gnu-inline-asm", but if we do that, I don't see a need to keep "-fno-inline-asm". If someone really wants to prevent both GNU and Microsoft-style inline asm, they can just specify -fno-gnu-inline-asm and -fno-asm-blocks.

steven_wu updated this revision to Diff 18318.Jan 16 2015, 1:52 PM
  • Remove -fno-inline-asm option and do not disallow asm label

    I agree that it is not a good idea disallow all asm labels with the new option. It is probably a Sema check should be implemented later to distinguish different asm labels and the type of occurrence (like in a system header).

    I don't like the part of code using -fno-inline-asm to imply the other two very much as well. I have no problem removing that flag.
echristo accepted this revision.Jan 16 2015, 2:22 PM
echristo added a reviewer: echristo.

Couple small inline nits, otherwise LGTM.

-eric

lib/Driver/Tools.cpp
4326

Period at the end of the sentence.

lib/Parse/ParseStmtAsm.cpp
619

Periods at the end of sentences.

test/Parser/ms-inline-asm.c
3

"Disabling gnu inline assembly should have no effect on this testcase."

This revision was automatically updated to reflect the committed changes.

Thanks. committed in r226340.

Steven

test/Driver/inline-asm.c