Page MenuHomePhabricator

[CodeGen][ARM] Error when writing to specific reserved registers in inline asm
ClosedPublic

Authored by vhscampos on Mar 26 2020, 7:26 AM.

Details

Summary

No error or warning is emitted when specific reserved registers are
written to in inline assembly. Therefore, writes to the program counter
or to the frame pointer, for instance, were permitted, which could have
led to undesirable behaviour.

Example:

int foo() {
  register int a __asm__("r7"); // r7 = frame-pointer in M-class ARM
  __asm__ __volatile__("mov %0, r1" : "=r"(a) : : );
  return a;
}

In contrast, GCC issues an error in the same scenario.

This patch detects writes to specific reserved registers in inline
assembly for ARM and emits an error in such case. The detection works
for output and input operands. Clobber operands are not handled here:
they are already covered at a later point in
AsmPrinter::emitInlineAsm(const MachineInstr *MI). The registers
covered are: program counter, frame pointer and base pointer.

This is ARM only. Therefore the implementation of other targets'
counterparts remain open to do.

Diff Detail

Event Timeline

vhscampos created this revision.Mar 26 2020, 7:26 AM
vhscampos updated this revision to Diff 252856.Mar 26 2020, 8:23 AM

Fixing naming issue detected by clang-tidy.

This seems to be diagnosing more cases than gcc; is that intentional?

It is diagnosing the same cases as the similar check present in CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp, lines 549-585. The latter, however, only covers clobber operands, leaving input and output operands unchecked.

I created these diagnostics in SelectionDAGBuilder.cpp, rather than in AsmPrinterInlineAsm.cpp, because, in the latter, it's too late to catch the case where input operands are reserved registers (i.e. inline asm's "local register variables").

It may diagnose more cases than GCC, but that is simply incidental as I just tried to mimick what is done with clobber registers.

I'd like to see a more complete comparison of clang vs. gcc with this patch. What's the effect on input operands? On output operands? On registers that are non-allocatable for different reasons?

I should have given more context, so let me amend that.

This patch is a fix for this bug here: https://bugs.llvm.org/show_bug.cgi?id=34165
LLVM does not warn (or error out) when inline asm is used to overwrite the frame pointer. This is problematic when the frame pointer is not omitted.

There's one exception though. When the frame pointer is in the clobber operand list, a warning is printed. This leaves the remaining two cases missing: output operand and input operand.

I decided to cover the entire range of non-clobberable registers because:

  1. the infrastrucuture for it was already there. Detecting only the frame pointer would involve more code than simply covering all non-clobberable registers (create new functions, etc).
  2. the already present detection, which only catches clobber operands, also covers all the non-clobberable registers.
  3. GCC seems to do this. E.g. it also catches writes to the 'pc' register.

Nevertheless, I have absolutely no objections to reduce the scope to just the frame pointer if you think that would be wiser.

I look forward to read your remarks.

I will paste here some other examples and the output of GCC for them.

Example 1:

void foo() {
  register int a __asm__("r7") = 54;
  __asm__ __volatile__("mov r0, %0": : "r"(a) : "r0");
}

GCC output:

teste.c: In function ‘foo’:
teste.c:4:1: error: r7 cannot be used in asm here
 }
 ^

Clang output:

teste.c:3:24: error: write to reserved register 'R7'
  __asm__ __volatile__("mov r0, %0" : : "r"(a) : "r0");
                       ^

Clang unpatched output: no error or warning

Example 2:

void foo() {
  register int a __asm__("r7");
  __asm__ __volatile__("mov %0, r1": "=r"(a) : :);
}

GCC output:

teste.c: In function ‘foo’:
teste.c:4:1: error: r7 cannot be used in asm here
 }
 ^

Clang output:

teste.c:3:24: error: write to reserved register 'R7'
  __asm__ __volatile__("mov r0, %0" : : "r"(a) : "r0");
                       ^

Clang unpatched output: no error or warning

Example 3:

void foo() {
  register int a __asm__("pc");
  __asm__ __volatile__("mov %0, r0" : "=r"(a) : : );
}

GCC output:

teste.c: In function ‘foo’:
teste.c:2:16: error: the register specified for ‘a’ is not general enough to be used as a register variable
   register int a __asm__("pc");
                ^

Clang output:

teste.c:3:24: error: write to reserved register 'PC'
  __asm__ __volatile__("mov r0, %0" : : "r"(a) : "r0");
                       ^

Clang unpatched output: no error or warning

Example 4:

void foo() {
  register int a __asm__("pc") = 54;
  __asm__ __volatile__("mov r0, %0" : : "r"(a) : "r0");
}

GCC output:

teste.c: In function ‘foo’:
teste.c:2:16: error: the register specified for ‘a’ is not general enough to be used as a register variable
   register int a __asm__("pc") = 54;
                ^

Clang output:

teste.c:3:24: error: write to reserved register 'PC'
  __asm__ __volatile__("mov r0, %0" : : "r"(a) : "r0");
                       ^

Clang unpatched output: no error or warning

Okay, thanks for the examples.

I tried a few more examples myself, and it looks like on ARM, gcc has some sort of special case for "sp", specifically?

vhscampos added a comment.EditedMar 30 2020, 5:13 AM

Indeed, it seems that gcc allows the use of SP in inline asm.

I could do one of the three:

  1. keep the SP register forbidden.
  2. have the SP as one exception.
  3. redo the patch to prevent only the use of the frame pointer.

Honestly I cannot come to a conclusion on which is the best option. Do you have an opinion on this?

Forbidding sp references would probably going to break real code for no reason; probably not a good idea.

Probably we want to try to follow gcc for whatever other obscure edge cases exist. Off the top of my head, the only other one on ARM is "fixed" registers. For example, with -ffixed-r4, gcc allows referencing r4.

What does that mean in terms of whitelisting/blacklisting? Probably makes sense to blacklist, if we're just talking about forbidding pc, the frame pointer, and the base pointer.

vhscampos updated this revision to Diff 254452.Apr 2 2020, 1:53 AM

The patch has been redone to detect only writes to PC, base pointer and
frame pointer on ARM target. Similar implementations can be done for
other targets as the base class's function is virtual.

vhscampos retitled this revision from [CodeGen] Error when writing to reserved registers in inline asm to [CodeGen][ARM] Error when writing to specific reserved registers in inline asm.Apr 2 2020, 2:00 AM
vhscampos edited the summary of this revision. (Show Details)

Are you intentionally ignoring clobbers?

Otherwise looks fine.

vhscampos updated this revision to Diff 255967.Apr 8 2020, 4:55 AM

Amended commit msg to explain why clobber operands are not handled.

vhscampos edited the summary of this revision. (Show Details)Apr 8 2020, 4:55 AM

Clobber operands are intentionally not handled in this patch. They were already covered in AsmPrinter::emitInlineAsm(const MachineInstr *MI), even though, in the latter, only a warning is given, not an error.

This revision is now accepted and ready to land.Apr 8 2020, 11:43 AM
This revision was automatically updated to reflect the committed changes.

The test in this commit writes to a local file (which it ignores). This makes it so the test can't run on a read-only file system. I have committed bf94c960071d338b7157ac7dee8120df50d5600f to fix.

Hi, I noticed this caught an error in chromium code that uses "r7": https://github.com/v8/v8/blob/502602923bf13d354a1b1f5fffdb63f13ab399a4/src/codegen/arm/cpu-arm.cc#L43
Was this change intended to break this use case?

It looks like you're overwriting the frame pointer (r7 in Thumb mode), which is not something LLVM supports in general. (This has never been supported, you just weren't getting a diagnostic before.) The diagnostic should only be enabled when the function actually has a frame pointer in r7; if you use -fomit-frame-pointer, it should compile successfully.

For that exact function, you probably get lucky on older versions of LLVM because the backend doesn't have any reason to use the frame pointer for anything; it's only getting emitted because of -fno-omit-frame-pointer or something like that.

In theory, we could teach the backend to automatically save/restore the frame pointer around inline asm in cases like this, instead of printing an error. Not sure what the details would look like.

Oh, ok. Thanks for the explanation!

I believe chromium's version is a copy of compiler-rt's implementation of clear_cache [1] .
@vhscampos Can you please fix compiler-rt version as well?

#define __ARM_NR_cacheflush 0x0f0002
  register int start_reg __asm("r0") = (int)(intptr_t)start;
  const register int end_reg __asm("r1") = (int)(intptr_t)end;
  const register int flags __asm("r2") = 0;
  const register int syscall_nr __asm("r7") = __ARM_NR_cacheflush;
  __asm __volatile("svc 0x0"
                   : "=r"(start_reg)
                   : "r"(syscall_nr), "r"(start_reg), "r"(end_reg), "r"(flags));

[1]: compiler-rt/lib/builtins/clear_cache.c

Hi @manojgupta. I am not familiar enough with this piece of code to understand whether it could be fixed by simply using any other general-purpose register instead of r7.

Can you request it to someone more accustomed to compiler-rt?

@vhscampos This change is more restrictive than GCC. e.g. https://godbolt.org/z/wP8_ic where clang errors out but GCC does not error out.

Please fix.

Hi @manojgupta. I am not familiar enough with this piece of code to understand whether it could be fixed by simply using any other general-purpose register instead of r7.

Can you request it to someone more accustomed to compiler-rt?

@vhscampos afaik, if anyone adds a new check that affects LLVM/subprojects, they should also take responsibility to clean up any internal LLVM issues unless there are volunteers.

This change isn't more restrictive than GCC. In your example, the difference is being caused by gcc seemingly adding -fomit-frame-pointer, whereas Clang does not.

@efriedma, could you share your view on this?

About compiler-rt, fair enough. I will work on it.

@manojgupta can you give a build recipe? None of the buildbots are failing.

manojgupta added a comment.EditedApr 28 2020, 1:33 PM

@manojgupta can you give a build recipe? None of the buildbots are failing.

Here is how I cam able to replicate on a debian :

1. Install gcc/g++ for arm gnueabi
$ sudo apt-get install  gcc-9-arm-linux-gnueabi  g++-9-arm-linux-gnueabi

2. build clang locally and create armv7a-linux-gnueabi-clang/clang++ symlinks
$ cd /path/to/build/bin/ 
$ ln -s clang++ armv7a-linux-gnueabi-clang++ 
$ ln -s clang armv7a-linux-gnueabi-clang

3. do a compiler-rt standalone build
$ mkdir /path/to/compiler-rt-build && cd  /path/to/compiler-rt-build
$ CFLAGS="-mthumb -I/usr/arm-linux-gnueabi/include/" CXXFLAGS="-mthumb -I/usr/arm-linux-gnueabi/include/c++/9/arm-linux-gnueabi/" CC= /path/to/build/bin/armv7a-linux-gnueabi-clang CXX= /path/to/build/bin/armv7a-linux-gnueabi-clang++ cmake -GNinja "-DCOMPILER_RT_TEST_TARGET_TRIPLE=arm-linux-gnueabi" -DCOMPILER_RT_DEBUG=ON /path/to/llvm-project/compiler-rt
$ ninja builtins -> fails with:

 llvm-project/compiler-rt/lib/builtins/clear_cache.c:80:20: error: write to reserved register 'R7'
  __asm __volatile("svc 0x0"
                   ^
1 error generated.

Regarding buildbots, they may not be using build config (thumb/gnueabi/COMPILER_RT_DEBUG=ON etc.) so thats why they may not see this problem.

@vhscampos This change is more restrictive than GCC. e.g. https://godbolt.org/z/wP8_ic where clang errors out but GCC does not error out.

This change isn't more restrictive than GCC. In your example, the difference is being caused by gcc seemingly adding -fomit-frame-pointer, whereas Clang does not.

But r7 was not an output, it's an input. We didn't write to it; we read from it. Regardless of frame pointers, aren't we still producing an error about writing to a register that we did not write to?

But r7 was not an output, it's an input. We didn't write to it; we read from it. Regardless of frame pointers, aren't we still producing an error about writing to a register that we did not write to?

An inline asm itself doesn't write to its inputs, but the compiler does. The compiler has to write the value of each input to the named location before the inline asm executes.