Bug 82435 - new __attribute__((alias)) warning gets in the way
Summary: new __attribute__((alias)) warning gets in the way
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: 8.0
: P3 normal
Target Milestone: ---
Assignee: Martin Sebor
URL:
Keywords: diagnostic
Depends on: 81854
Blocks:
  Show dependency treegraph
 
Reported: 2017-10-05 10:11 UTC by Arnd Bergmann
Modified: 2017-10-13 11:18 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-10-05 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Arnd Bergmann 2017-10-05 10:11:12 UTC
gcc-8 started warning about function aliases that have a non-matching prototype. This seems rather useful in general, but it causes tons of warnings in the Linux kernel, where we rely on abusing those aliases for system call entry points, in order to sanitze the arguments passed from user space in registers, see 
http://elixir.free-electrons.com/linux/v4.13/source/include/linux/syscalls.h#L84

In file included from /git/arm-soc/kernel/sysctl_binary.c:6:0:
/git/arm-soc/include/linux/syscalls.h:211:18: error: 'sys_sysctl' alias between functions of incompatible types 'long int(struct __sysctl_args *)' and 'long int(long int)' [-Werror=attributes]
  asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
                  ^~~
/git/arm-soc/include/linux/syscalls.h:207:2: note: in expansion of macro '__SYSCALL_DEFINEx'
  __SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
  ^~~~~~~~~~~~~~~~~
/git/arm-soc/include/linux/syscalls.h:196:36: note: in expansion of macro 'SYSCALL_DEFINEx'
 #define SYSCALL_DEFINE1(name, ...) SYSCALL_DEFINEx(1, _##name, __VA_ARGS__)
                                    ^~~~~~~~~~~~~~~
/git/arm-soc/kernel/sysctl_binary.c:1418:1: note: in expansion of macro 'SYSCALL_DEFINE1'
 SYSCALL_DEFINE1(sysctl, struct __sysctl_args __user *, args)
 ^~~~~~~~~~~~~~~
/git/arm-soc/include/linux/syscalls.h:215:18: note: aliased declaration here
  asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
                  ^~~
/git/arm-soc/include/linux/syscalls.h:207:2: note: in expansion of macro '__SYSCALL_DEFINEx'
  __SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
  ^~~~~~~~~~~~~~~~~
/git/arm-soc/include/linux/syscalls.h:196:36: note: in expansion of macro 'SYSCALL_DEFINEx'
 #define SYSCALL_DEFINE1(name, ...) SYSCALL_DEFINEx(1, _##name, __VA_ARGS__)
                                    ^~~~~~~~~~~~~~~
/git/arm-soc/kernel/sysctl_binary.c:1418:1: note: in expansion of macro 'SYSCALL_DEFINE1'
 SYSCALL_DEFINE1(sysctl, struct __sysctl_args __user *, args)
 ^~~~~~~~~~~~~~~

I have not found a way to disable this warning without also disabling all other warnings about attributes. Ideally we'd have another attribute that lets us keep on using the existing kernel code with macro to pick the right attributes based on the gcc version. Alternatively, a separate diagnostic flag to turn off the warning about incompatible aliases globally would also work.
Comment 1 Martin Sebor 2017-10-05 14:57:58 UTC
The warning can be selectively suppressed by declaring the alias to have no prototype, e.g., like so:

  int bar (int i) { return 0; }
  int foo () __attribute__ ((alias ("bar")));

In the latest revision of the patch (still under review), the warning is controlled by -Wincompatible-pointer-types rather than -Wattributes.

Is this sufficient?
Comment 2 joseph@codesourcery.com 2017-10-05 16:25:19 UTC
In my view, a separate option for this warning rather than 
-Wincompatible-pointer-types would be best (as it's much more likely 
people will want to disable this warning than that they will want to 
disable other cases of -Wincompatible-pointer-types or -Wattributes).
Comment 3 Arnd Bergmann 2017-10-05 20:51:21 UTC
(In reply to Martin Sebor from comment #1)
> The warning can be selectively suppressed by declaring the alias to have no
> prototype, e.g., like so:
> 
>   int bar (int i) { return 0; }
>   int foo () __attribute__ ((alias ("bar")));

I think this won't work since we rely both prototypes to be just what they are (yes, this is an ugly area of the kernel)

> In the latest revision of the patch (still under review), the warning is
> controlled by -Wincompatible-pointer-types rather than -Wattributes.
> 
> Is this sufficient?

Maybe we could change the SYSCALL_DEFINEx macro to use _Pragma("GCC diagnostic push") etc to turn off whichever warning we hit around just the alias declaration. Having a new warning to turn off globally would be easier though.
Comment 4 Bernd Edlinger 2017-10-06 10:39:20 UTC
Arnd, excuse me for possibly silly question.

Why is it necessary to have
sys_bla(with correct paramters)
and
SyS_bla(with generic parameters)

On the ABI level these aliases should be equivalent,
or are these not really the same?
Comment 5 Arnd Bergmann 2017-10-06 10:51:12 UTC
(In reply to Bernd Edlinger from comment #4)
> Arnd, excuse me for possibly silly question.
> 
> Why is it necessary to have
> sys_bla(with correct paramters)
> and
> SyS_bla(with generic parameters)
> 
> On the ABI level these aliases should be equivalent,
> or are these not really the same?

The first one (sys_*) matches the prototype that is defined in a
header file and matches whatever user space should see. It can also
be called from other parts of the kernel using that prototype. This
is an alias to the second.

The second one (SyS_*) uses 'unsigned long' arguments to
ensure we don't get undefined behavior with arguments that
are shorter than a CPU register: When the ELF psABI calling
conventions mandate that the upper bits in a register are
zero-extended or sign-extended, calling the function from user
space with intentionally invalid contents may cause undefined
behavior, such as overflowing an array indexed by an 'unsigned
char' argument. The typecast from unsigned long to the real
type should ensure that the upper bits are correctly initialized
on all supported ABIs.

The third (SYS_*) prototype is for the inline function that contains
the actual syscall implementation, and it again has the correct
prototype according to the header file.
Comment 6 Martin Sebor 2017-10-06 15:17:35 UTC
Thanks for the background.  It's unfortunate that the kernel chose to exploit one kind of undefined behavior in order to protect itself from another.  From your description it doesn't sound to me like it's going to be possible to change the macros to declare the aliases with matching signatures and have the kernel benefit from the type checking.  A new option to suppress the warning might be the next best solution.  A variant to consider might be to make it a two-level warning to allow this sort of abuse while still detecting other mismatches.  I don't know if it's worth the effort.

Let me confirm this bug as a request to add a new option to control the warning.
Comment 7 Martin Sebor 2017-10-11 16:22:56 UTC
I've added a new option (-Wattribute-alias) in the latest update to my patch for bug 82301:
https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00678.html
Comment 8 Martin Sebor 2017-10-12 17:38:27 UTC
Author: msebor
Date: Thu Oct 12 17:37:56 2017
New Revision: 253688

URL: https://gcc.gnu.org/viewcvs?rev=253688&root=gcc&view=rev
Log:
PR c/82301 - Updated test case g++.dg/ext/attr-ifunc-1.C (and others) in r253041 segfault on powerpc64
PR c/82435 - new __attribute__((alias)) warning gets in the way

gcc/ChangeLog:

	PR other/82301
	PR c/82435
	* cgraphunit.c (maybe_diag_incompatible_alias): New function.
	(handle_alias_pairs): Call it.
	* common.opt (-Wattribute-alias): New option.
	* doc/extend.texi (ifunc attribute): Discuss C++ specifics.
	* doc/invoke.texi (-Wattribute-alias): Document.

gcc/testsuite/ChangeLog:

	PR other/82301
	PR c/82435
	* g++.dg/ext/attr-ifunc-1.C: Update.
	* g++.dg/ext/attr-ifunc-2.C: Same.
	* g++.dg/ext/attr-ifunc-3.C: Same.
	* g++.dg/ext/attr-ifunc-4.C: Same.
	* g++.dg/ext/attr-ifunc-5.C: Same.
	* g++.dg/ext/attr-ifunc-6.C: New test.
	* g++.old-deja/g++.abi/vtable2.C: Update.
	* gcc.dg/attr-ifunc-6.c: New test.
	* gcc.dg/attr-ifunc-7.c: New test.
	* gcc.dg/pr81854.c: Update.
	* lib/target-supports.exp: Update.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cgraphunit.c
    trunk/gcc/common.opt
    trunk/gcc/doc/extend.texi
    trunk/gcc/doc/invoke.texi
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/ext/attr-ifunc-1.C
    trunk/gcc/testsuite/g++.dg/ext/attr-ifunc-2.C
    trunk/gcc/testsuite/g++.dg/ext/attr-ifunc-3.C
    trunk/gcc/testsuite/g++.dg/ext/attr-ifunc-4.C
    trunk/gcc/testsuite/g++.dg/ext/attr-ifunc-5.C
    trunk/gcc/testsuite/g++.old-deja/g++.abi/vtable2.C
    trunk/gcc/testsuite/gcc.dg/pr81854.c
    trunk/gcc/testsuite/lib/target-supports.exp
Comment 9 Martin Sebor 2017-10-12 17:41:07 UTC
The type checking is now controlled by a dedicated warning (-Wattribute-alias) committed in r253688.
Comment 10 Arnd Bergmann 2017-10-13 11:18:58 UTC
I get a clean kernel build now with that option, thanks a lot for your help!