Bug 86673 - [8/9 regression] inline asm sometimes ignores 'register asm("reg")' declarations
Summary: [8/9 regression] inline asm sometimes ignores 'register asm("reg")' declarations
Status: RESOLVED DUPLICATE of bug 85745
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 8.1.1
: P3 normal
Target Milestone: ---
Assignee: Thomas Preud'homme
URL:
Keywords: assemble-failure
Depends on:
Blocks:
 
Reported: 2018-07-25 12:23 UTC by Arnd Bergmann
Modified: 2018-07-30 12:27 UTC (History)
4 users (show)

See Also:
Host:
Target: arm-none-linux-gnueabi , arm-none-eabi
Build:
Known to work: 7.2.0
Known to fail: 8.1.0, 9.0
Last reconfirmed: 2018-07-25 00:00:00


Attachments
linux/net/core/scm.o, preprocessed (260.87 KB, application/gzip)
2018-07-25 12:23 UTC, Arnd Bergmann
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Arnd Bergmann 2018-07-25 12:23:41 UTC
Created attachment 44438 [details]
linux/net/core/scm.o, preprocessed

Building older linux kernels for ARM with a gcc-8.1 compiler has triggered a check for broken compiler versions, which compares the register number that is used in an inline assembly statement with the expected value, for an argument that was declared with the 'register foo asm ("reg")' syntax described in the gcc manual under "Specifying Registers for Local Variables".

The diagnostic from the assembler is

$ arm-linux-gnueabi-gcc -Wall -O2 scm.i -c -Wno-pointer-sign -fno-strict-aliasing
/tmp/ccCGMQmS.s:648: Error: .err encountered
/tmp/ccCGMQmS.s:679: Error: .err encountered

Unfortunately, a change made to the kernel a few years ago had made this go unnoticed as everyone was testing gcc-8.1 only on more recent kernels that did not run into the particular check, but may have run into the bug without triggering the check. Architectures other than arm may also be affected, but nothing else has this check.

I tested gcc-8.1.0 and today's gcc-8.1.1 (r262956), both with the same result. I attached one of the files that showed the problem, and reduced this using creduce to:

int a, c, d, e;
long b;
void fn1() {
  int f = ({
    ({
      long g = b, j = g;
      register const typeof(c) h asm("r2") = 1, i = d;
      __asm__(".ifnc %2,r2; .err; .endif\n\t"
                "bl	__put_user_4"
              : "=&r"(e)
              : ""(i), ""(h), ""(j));
      e;
    });
  });
  a = f;
}
Comment 1 Arnd Bergmann 2018-07-25 12:43:44 UTC
Further inspection shows that this happens for the cases where the input argument to the inline asm is a compile-time constant, but not for those that are variables.
Comment 2 Arnd Bergmann 2018-07-25 12:55:15 UTC
Forcing constant inputs for put_user to be read from a volatile variable avoids this problem and lets me cleanly build all files that showed it.

diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 35c9db857ebe..23e92a9a5ef4 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -251,7 +251,8 @@ extern int __put_user_8(void *, unsigned long long);
        ({                                                              \
                unsigned long __limit = current_thread_info()->addr_limit - 1; \
                const typeof(*(p)) __user *__tmp_p = (p);               \
-               register const typeof(*(p)) __r2 asm("r2") = (x);       \
+               const typeof(*(p)) __x = (x);                           \
+               register const typeof(*(p)) __r2 asm("r2") = READ_ONCE(__x);    \
                register const typeof(*(p)) __user *__p asm("r0") = __tmp_p; \
                register unsigned long __l asm("r1") = __limit;         \
                register int __e asm("r0");                             \

This confirms that constant inputs are what leads to the problem.
Comment 3 Ramana Radhakrishnan 2018-07-25 13:02:26 UTC
Confirmed.
Comment 4 Andreas Schwab 2018-07-25 13:10:34 UTC
Why are you using empty constraints when a register is required?
Comment 5 Arnd Bergmann 2018-07-25 13:15:51 UTC
(In reply to Andreas Schwab from comment #4)
> Why are you using empty constraints when a register is required?

creduce did that, it had no effect on the result. The original source looks like:

#define __get_user_x_64t(__r2, __p, __e, __l, __s)                      \
           __asm__ __volatile__ (                                       \
                __asmeq("%0", "r0") __asmeq("%1", "r2")                 \
                __asmeq("%3", "r1")                                     \
                "bl     __get_user_64t_" #__s                           \
                : "=&r" (__e), "=r" (__r2)                              \
                : "0" (__p), "r" (__l)                                  \
                : __GUP_CLOBBER_##__s)
Comment 6 Thomas Preud'homme 2018-07-25 14:04:01 UTC
The following simpler testcase also shows the problem:

void fn1() {
  register const int h asm("r2") = 1;
  __asm__(".ifnc %0,r2; .err; .endif\n\t"
          "bl   __put_user_4" :: "r"(h));
}
Comment 7 Thomas Preud'homme 2018-07-25 14:07:53 UTC
(In reply to Thomas Preud'homme from comment #6)
> The following simpler testcase also shows the problem:
> 
> void fn1() {
>   register const int h asm("r2") = 1;
>   __asm__(".ifnc %0,r2; .err; .endif\n\t"
>           "bl   __put_user_4" :: "r"(h));
> }

The register label gets optimized during gimple stages.
Comment 8 Thomas Preud'homme 2018-07-25 14:13:56 UTC
(In reply to Thomas Preud'homme from comment #7)
> (In reply to Thomas Preud'homme from comment #6)
> > The following simpler testcase also shows the problem:
> > 
> > void fn1() {
> >   register const int h asm("r2") = 1;
> >   __asm__(".ifnc %0,r2; .err; .endif\n\t"
> >           "bl   __put_user_4" :: "r"(h));
> > }
> 
> The register label gets optimized during gimple stages.

Dump for original already shows propagation of the constant has happened which later lead to the removal of the register declaration:

;; Function fn1 (null)
;; enabled by -tree-original


{
  register const int h __asm__ (*r2) = 1;

    register const int h __asm__ (*r2) = 1;
  __asm__ __volatile__(".ifnc %0,r2; .err; .endif\n\tbl\t__put_user_4"::"r" 1);
}
Comment 9 Arnd Bergmann 2018-07-25 14:37:50 UTC
Reproduced on arm64 and x86 as well, see x86 version:

void fn1() {
   register const int h asm("edx") = 1;
    __asm__(".ifnc %0,edx; .err; .endif" :: "r"(h));
}
Comment 10 Alexander Monakov 2018-07-25 14:47:09 UTC
See PR 85745 where Jakub said,

The reason this happens is that the register variable is marked const.  Don't do that.  If it is const, the compiler optimizes it more aggressively - it will happily fold uses of the variable to the constant ininitializer, so the inline asm becomes "r" (110) instead of "r" (__r2) and thus it can use any register.
This is how C++ behaved for years and how C in GCC behaves since the folding improvements.
Comment 11 Arnd Bergmann 2018-07-25 15:29:10 UTC
I have checked all instances of 'register const' or 'const register' in the current linux kernel (4.18-rc), and we never assign a constant expression to any of them, so I guess none of them are affected:

arch/arm/include/asm/uaccess.h:		register const void __user *__p asm("r0") = __ptr;	\
arch/h8300/kernel/sim-console.c:	register const char *_ptr __asm__("er1") = s;
arch/h8300/kernel/sim-console.c:	register const unsigned _len __asm__("er2") = n;
arch/mips/include/asm/uaccess.h:	register const void __user *__cu_from_r __asm__("$5");		\
arch/mips/include/asm/uaccess.h:	register const void *__cu_from_r __asm__("$5");			\
arch/riscv/kernel/process.c:		const register unsigned long gp __asm__ ("gp");
arch/riscv/kernel/stacktrace.c:		const register unsigned long current_sp __asm__ ("sp");
arch/riscv/kernel/stacktrace.c:		const register unsigned long current_sp __asm__ ("sp");

Should we drop the 'const' for all of them as a rule? If there is no use case for ever using a 'const register' variable and it can lead to bugs, should gcc warn about it in the future?

Should this issue be mentioned in the documentation in https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html?

I also checked all instances in linux-4.4, and the ARM put_user() helper is the only one I see that gets a constant expression input, so I suppose that is all that needs to be fixed in backports, unless someone thinks we should get rid of all them in backports as well.
Comment 12 Andreas Schwab 2018-07-25 15:43:57 UTC
arch/h8300/kernel/sim-console.c:	register const int fd __asm__("er0") = 1; /* stdout */
Comment 13 Arnd Bergmann 2018-07-25 15:56:57 UTC
(In reply to Andreas Schwab from comment #12)
> arch/h8300/kernel/sim-console.c:	register const int fd __asm__("er0") = 1;

I found that too, and then noticed it is already fixed in linux-next:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=14cf9451be78f8a

Ard points out that most of the other ones are pointers to const data, which are not a problem. This leaves the arm put_user bug as the only definite problem that needs to be addressed in older kernels.

The three arch/riscv instances of 'const register unsigned long gp __asm__ ("gp")' are different because they are never passed into an inline assembly as far as I can tell. This seems to be unsupported for local register variables according to the gcc documentation, but if that's a problem, it's unrelated to this bug.
Comment 14 Alexander Monakov 2018-07-25 16:02:43 UTC
Indeed, I agree the documentation should mention that (I can write a patch). IIRC 'volatile register' also does not behave intuitively (I'll try to double check).

I'm unsure about a warning, it seems "doing it right" might require changes in both C and C++ frontends, and issuing a warning only if an asm statement using the register variable is present?

Adding Jakub to Cc.
Comment 15 Jakub Jelinek 2018-07-25 16:13:00 UTC
register const void __user *__p asm("r0") = __ptr;
etc. isn't a problem, here __p isn't const, only *__p is.  The problem is if the register variable itself is const.
So
arch/h8300/kernel/sim-console.c:	register const unsigned _len __asm__("er2") = n;
If n can be a constant or is const var initialized to constant etc.
The cases where a const register var doesn't have an initializer aren't a problem either.

I don't see what should we warn about.
Comment 16 Alexander Monakov 2018-07-25 16:47:43 UTC
> I don't see what should we warn about.

I think the suggestion was to warn when a register const variable appears in constraints, because the asm is not then guaranteed to receive that operand in that register, contrary to user's expectations? So basically warn about this non-intuitive aspect of local reg vars so the places where the code makes a wrong assumption can be identified and fixed.
Comment 17 Andrew Pinski 2018-07-25 17:29:11 UTC
Dup of bug 85745.

*** This bug has been marked as a duplicate of bug 85745 ***
Comment 18 Alexander Monakov 2018-07-30 12:27:08 UTC
Author: amonakov
Date: Mon Jul 30 12:26:37 2018
New Revision: 263065

URL: https://gcc.gnu.org/viewcvs?rev=263065&root=gcc&view=rev
Log:
doc: discourage const/volatile on register variables (PR 86673)

	PR target/86673
	* doc/extend.texi (Global Register Variables): Discourage use of type
	qualifiers.
	(Local Register Variables): Likewise.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/doc/extend.texi