Page MenuHomePhabricator

[CodeGen] Remove buggy handling of input operands in inline asm
Needs ReviewPublic

Authored by vhscampos on May 12 2020, 5:44 AM.



The detection of writes to reserved registers in inline asm cover the
three cases: input, output, or clobber operands. In the input case,
however, all uses are flagged as writes, even when the register is only
read from.

This patch removes the buggy part that handles input operands.

It seems that this case should be handled in clang instead, as the IR
(and codegen) stage is too late to properly detect writes to input

Diff Detail

Event Timeline

vhscampos created this revision.May 12 2020, 5:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2020, 5:44 AM

Ultimately, if we really want to "support" this, I expect the end result should look like the following:

  1. We keep this diagnostic.
  2. We introduce a new input type "read a fixed register", which doesn't have a corresponding IR operand.
  3. We teach clang to emit "read a fixed register" for appropriate inputs (maybe register variables which are never initialized or written).

Really, I'm not sure how useful the end result would be, though. This isn't providing any functionality that the user can't already get by just explicitly writing the register in the asm string. And gcc doesn't support equivalent functionality. So ultimately, I'm not sure what you're hoping to accomplish. Is there some existing code which is using this pattern?

This effort is to specifically address this bug:

void fn(int fd, int arg)
  register int r7 __asm__("r7") = 54;
  __asm__ __volatile__ ( "svc 0" : : "r"(r7), "r"(fd), "r"(arg) : "memory");

gcc does provide such functionality. For example, the code above errors out in gcc.

In the case of writing the fixed register in the string itself, gcc and clang are already equivalent.

  1. r7 is in the string and in the clobber list. Clang and gcc give a diagnostic message.
void foo() {
  volatile int a;
  __asm__ __volatile("mov r7, %0" : : "r"(a) : "r7");
  1. r7 is in the string, but not in the clobber list. Clang and gcc give no message.
void foo() {
  volatile int a;
  __asm__ __volatile("mov r7, %0" : : "r"(a) : );

I look forward to hearing your thoughts on this. Thanks

I looked at bug 34165, and concluded that clang and gcc behave the same way, ignoring the different default for -fomit-frame-pointer. Am I missing something?

vhscampos added a comment.EditedMay 28 2020, 6:05 AM

This is the problematic case:

void fn(int fd, int arg)
  register int r7 __asm__("r7");
  __asm__ __volatile__ ( "svc 0" : : "r"(r7), "r"(fd), "r"(arg) : "memory");

Note that, differently from bug 34165, 'r7' is not written to here.

gcc gives no warning or error. Clang, however:

b.c:4:26: error: write to reserved register 'R7'
  __asm__ __volatile__ ( "svc 0" : : "r"(r7), "r"(fd), "r"(arg) : "memory");

And this is because the current code considers as an error any use of 'r7' as input operand, regardless of it being written to or not.

A correct implementation should trigger an error only when the input operand is written to. This is what motivates me to remove the handling of input operands while we come up with a correct implementation, likely in Clang rather than LLVM as you suggested.

The handling of output operands will remain present as it has not been shown to be wrong.

Now I understand better what you meant: the current code will become correct once we make modifications in Clang to mark read-only input register operands as having no value operands at all.

I believe then it is a matter of deciding whether or not to remove the current input operand handling while these modifications are worked on.

Oh, hmm, somehow I missed that when I looked at this before; gcc actually tracks whether the variable is initialized/assigned-to, and doesn't emit an error if there is no assignment. And, as usual with gcc, whether the error is emitted depends on the optimization level in some cases. That seems completely bizarre, but okay, I guess.

If there's some real code depending on that, sure, we can temporarily revert the relevant LLVM changes.

That said, nobody has mentioned the uninitialized variable pattern showing up in real code; do you have an example?