Bug 21029 - glibc-2.23 (and later) fails to compile with -fno-omit-frame-pointer on i386
Summary: glibc-2.23 (and later) fails to compile with -fno-omit-frame-pointer on i386
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: 2.23
: P1 normal
Target Milestone: 2.26
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-06 10:10 UTC by Jan Ziak (http://atom-symbol.net)
Modified: 2017-02-18 14:32 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2017-01-07 00:00:00
fweimer: security-


Attachments
Gentoo build log: sys-libs:glibc-2.23-r3:20170106-093416.log (84.67 KB, text/x-log)
2017-01-06 10:10 UTC, Jan Ziak (http://atom-symbol.net)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Ziak (http://atom-symbol.net) 2017-01-06 10:10:34 UTC
Created attachment 9741 [details]
Gentoo build log: sys-libs:glibc-2.23-r3:20170106-093416.log

I am compiling all packages on my machines with -fno-omit-frame-pointer because it enables debuggers and performance measurement tools (gdb, perf record, valgrind memcheck, valgrind callgrind).

However, glibc-2.23 fails to compile on my system:

../sysdeps/unix/sysv/linux/posix_fallocate.c: In function 'posix_fallocate':
../sysdeps/unix/sysv/linux/posix_fallocate.c:39:1: error: bp cannot be used in asm here

../sysdeps/unix/sysv/linux/posix_fallocate64.c: In function '__posix_fallocate64_l64':
../sysdeps/unix/sysv/linux/posix_fallocate64.c:42:1: error: bp cannot be used in asm here

This bug is most likely caused by sysdeps/unix/sysv/linux/i386/sysdep.h:570:

# define LOADREGS_6(arg1, arg2, arg3, arg4, arg5, arg6) \
        register unsigned int _a6 asm ("ebp") = (unsigned int) (arg6); \
        LOADREGS_5 (arg1, arg2, arg3, arg4, arg5)
Comment 1 Adhemerval Zanella 2017-01-06 20:11:08 UTC
AFAIK it is not an issue because 'ebp' is this context is used on kernel ABI to pass the 6th argument (the file sysdeps/unix/sysv/linux/i386/sysdep.h contains a comment about the linux convention for i686).

Unless there is a compiler option to make easier to create syscall macros (so compiler can restore ebp somehow), you will need to explict disable -fno-omit-frame-pointer on these files.

Another option would to code this syscalls in asm, but I will advise against since to make i386 to use the generic code is exactly to avoid the proliferation of such implementations.
Comment 2 Andreas Schwab 2017-01-06 21:06:03 UTC
Fixed in commit 6b1df8b27f.

*** This bug has been marked as a duplicate of bug 20729 ***
Comment 3 Jan Ziak (http://atom-symbol.net) 2017-01-07 10:15:18 UTC
(In reply to Adhemerval Zanella from comment #1)
> AFAIK it is not an issue because 'ebp' is this context is used on kernel ABI
> to pass the 6th argument (the file sysdeps/unix/sysv/linux/i386/sysdep.h
> contains a comment about the linux convention for i686).

It's an issue as long as it is fixable in glibc. The fact is, it *is* fixable in glibc.

It's an issue as long as it is fixable in gcc. The fact is, it *is* fixable in gcc.

It's an issue as long as it is fixable in the Linux kernel. The fact is, it *is* fixable in Linux kernel.

It's an issue as long as it is fixable on my machine alone. The fact is, it *is* fixable on my machine alone.

The question is: which software component is the easiest to change and will have the most impact on the world.

> Unless there is a compiler option to make easier to create syscall macros
> (so compiler can restore ebp somehow), you will need to explict disable
> -fno-omit-frame-pointer on these files.
> 
> Another option would to code this syscalls in asm, but I will advise against
> since to make i386 to use the generic code is exactly to avoid the
> proliferation of such implementations.

The best solution is the update the definition of INTERNAL_SYSCALL with something like this:

    LOADREGS_##nr(args)
    asm volatile (
    PUSHREGS_ASM_##nr(args)
    "call *%%gs:%P2"
    POPREGS_ASM_##nr
    : "=a" (resultvar)

#define PUSHREGS_ASM_5(args...) "" // empty
#define PUSHREGS_ASM_6(args...) "push %%ebp; mov arg6, %%ebp;"

#define POPREGS_ASM_5 "" // empty
#define POPREGS_ASM_6 "pop %%ebp"
Comment 4 Jan Ziak (http://atom-symbol.net) 2017-01-07 10:19:25 UTC
(In reply to Andreas Schwab from comment #2)
> Fixed in commit 6b1df8b27f.
> 
> *** This bug has been marked as a duplicate of bug 20729 ***

https://sourceware.org/git/gitweb.cgi?p=glibc.git;a=commitdiff;h=6b1df8b27f

6b1df8b27f is inferior to the solution I provided in my previous comment. Please revert 6b1df8b27f and commit a patch based on my proposal. Thanks.
Comment 5 Jan Ziak (http://atom-symbol.net) 2017-01-07 10:53:16 UTC
(In reply to Jan Ziak (http://atom-symbol.net) from comment #3)
> The best solution is the update the definition of INTERNAL_SYSCALL with
> something like this:
> 
>     LOADREGS_##nr(args)
>     asm volatile (
>     PUSHREGS_ASM_##nr(args)
>     "call *%%gs:%P2"
>     POPREGS_ASM_##nr
>     : "=a" (resultvar)
> 
> #define PUSHREGS_ASM_5(args...) "" // empty
> #define PUSHREGS_ASM_6(args...) "push %%ebp; mov arg6, %%ebp;"
> 
> #define POPREGS_ASM_5 "" // empty
> #define POPREGS_ASM_6 "pop %%ebp"

#define PUSHREGS_ASM_6(args...) "pushl arg6; push %%ebp; mov 4(%%esp), %%ebp;"

#define POPREGS_ASM_6 "pop %%ebp; add $4, %%esp"
Comment 6 Adhemerval Zanella 2017-01-07 13:35:35 UTC
(In reply to Jan Ziak (http://atom-symbol.net) from comment #3)
> (In reply to Adhemerval Zanella from comment #1)
> > AFAIK it is not an issue because 'ebp' is this context is used on kernel ABI
> > to pass the 6th argument (the file sysdeps/unix/sysv/linux/i386/sysdep.h
> > contains a comment about the linux convention for i686).
> 
> It's an issue as long as it is fixable in glibc. The fact is, it *is*
> fixable in glibc.
> 
> It's an issue as long as it is fixable in gcc. The fact is, it *is* fixable
> in gcc.
> 
> It's an issue as long as it is fixable in the Linux kernel. The fact is, it
> *is* fixable in Linux kernel.
> 
> It's an issue as long as it is fixable on my machine alone. The fact is, it
> *is* fixable on my machine alone.
> 
> The question is: which software component is the easiest to change and will
> have the most impact on the world.

It was a bad wording from my part, my idea is since we already have a solution for correct 6-argument passing for compiler that might use the 6th argument passing it should not be an issue on GLIBC.

However I wrongly assumed this could not be accomplished on this code, which it wrong.

> 
> > Unless there is a compiler option to make easier to create syscall macros
> > (so compiler can restore ebp somehow), you will need to explict disable
> > -fno-omit-frame-pointer on these files.
> > 
> > Another option would to code this syscalls in asm, but I will advise against
> > since to make i386 to use the generic code is exactly to avoid the
> > proliferation of such implementations.
> 
> The best solution is the update the definition of INTERNAL_SYSCALL with
> something like this:
> 
>     LOADREGS_##nr(args)
>     asm volatile (
>     PUSHREGS_ASM_##nr(args)
>     "call *%%gs:%P2"
>     POPREGS_ASM_##nr
>     : "=a" (resultvar)
> 
> #define PUSHREGS_ASM_5(args...) "" // empty
> #define PUSHREGS_ASM_6(args...) "push %%ebp; mov arg6, %%ebp;"
> 
> #define POPREGS_ASM_5 "" // empty
> #define POPREGS_ASM_6 "pop %%ebp"

Your solution is wrong and do not address the issue in question.  This snippet is not related to C macros for syscall macros, but rather to auto-generated syscalls which is code and implemented directly in assembly.

Six argument syscalls from C macros were fixed by a9fe4c5aa8e53ee3 and then optimized on 98ad631cd0a77.  The optimization takes in consideration than if you are using GCC 5, the compiler an properly spill %ebx when needed, we can inline syscalls with 6 arguments.

The problem is it does not take in consideration that for fomit-frame-pointer compiler option on i686 we can not use this optimization (as for PROF which was corrected by 95b097779a6).

I could get a working build with the scratch patch:

-----

diff --git a/sysdeps/unix/sysv/linux/i386/Makefile b/sysdeps/unix/sysv/linux/i386/Makefile
index 9609752..cd5bb43 100644
--- a/sysdeps/unix/sysv/linux/i386/Makefile
+++ b/sysdeps/unix/sysv/linux/i386/Makefile
@@ -3,7 +3,7 @@ default-abi := 32
 
 # %ebp is used to pass the 6th argument to system calls, so these
 # system calls are incompatible with a frame pointer.
-uses-6-syscall-arguments = -fomit-frame-pointer
+uses-6-syscall-arguments = -DNO_OPTIMIZE_FOR_GCC_5 
 
 ifeq ($(subdir),misc)
 sysdep_routines += ioperm iopl vm86
@@ -24,6 +24,7 @@ CFLAGS-semtimedop.os += $(uses-6-syscall-arguments)
 endif
 
 ifeq ($(subdir),elf)
+sysdep-dl-routines += libc-do-syscall
 sysdep-others += lddlibc4
 install-bin += lddlibc4
 endif
@@ -61,6 +62,8 @@ ifeq ($(subdir),nptl)
 # pull in __syscall_error routine
 libpthread-routines += sysdep
 libpthread-shared-only-routines += sysdep
+CFLAGS-pthread_cond_wait.o += $(uses-6-syscall-arguments)
+CFLAGS-pthread_cond_wait.os += $(uses-6-syscall-arguments)
 CFLAGS-pthread_rwlock_timedrdlock.o += $(uses-6-syscall-arguments)
 CFLAGS-pthread_rwlock_timedrdlock.os += $(uses-6-syscall-arguments)
 CFLAGS-pthread_rwlock_timedwrlock.o += $(uses-6-syscall-arguments)
diff --git a/sysdeps/unix/sysv/linux/i386/libc-do-syscall.S b/sysdeps/unix/sysv/linux/i386/libc-do-syscall.S
index f706c6d..ef9e513 100644
--- a/sysdeps/unix/sysv/linux/i386/libc-do-syscall.S
+++ b/sysdeps/unix/sysv/linux/i386/libc-do-syscall.S
@@ -18,7 +18,7 @@
 
 #include <sysdep.h>
 
-#ifndef OPTIMIZE_FOR_GCC_5
+//#ifndef OPTIMIZE_FOR_GCC_5
 
 /* %eax, %ecx, %edx and %esi contain the values expected by the kernel.
    %edi points to a structure with the values of %ebx, %edi and %ebp.  */
@@ -50,4 +50,4 @@ ENTRY (__libc_do_syscall)
 	cfi_restore (ebx)
 	ret
 END (__libc_do_syscall)
-#endif
+//#endif
diff --git a/sysdeps/unix/sysv/linux/i386/sysdep.h b/sysdeps/unix/sysv/linux/i386/sysdep.h
index baf4642..e35cc02 100644
--- a/sysdeps/unix/sysv/linux/i386/sysdep.h
+++ b/sysdeps/unix/sysv/linux/i386/sysdep.h
@@ -46,7 +46,7 @@
    to compile glibc.  Disable GCC 5 optimization when compiling for
    profiling since asm ("ebp") can't be used to put the 6th argument
    in %ebp for syscall.  */
-#if __GNUC_PREREQ (5,0) && !defined PROF
+#if __GNUC_PREREQ (5,0) && !defined PROF && !defined NO_OPTIMIZE_FOR_GCC_5
 # define OPTIMIZE_FOR_GCC_5
 #endif
 
-----

It is not optimal since we want NO_OPTIMIZE_FOR_GCC_5 only when omit-frame-pointer is used and it will require most likely an configure check to do so. I will think in a better solution.
Comment 7 Jan Ziak (http://atom-symbol.net) 2017-01-11 08:49:49 UTC
(In reply to Adhemerval Zanella from comment #6)
> The optimization takes in consideration than if
> you are using GCC 5, the compiler an properly spill %ebx when needed, we can
> inline syscalls with 6 arguments.

Just a note: stack frame pointer is stored in %ebp, not %ebx.
Comment 8 cvs-commit@gcc.gnu.org 2017-02-17 20:07:36 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  3b33d6ed6096c1d20d05a650b06026d673f7399a (commit)
      from  52ac22365a332cacf7aa97f1b41b3a0adfaff778 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=3b33d6ed6096c1d20d05a650b06026d673f7399a

commit 3b33d6ed6096c1d20d05a650b06026d673f7399a
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date:   Sun Jan 8 11:38:23 2017 -0200

    Rework -fno-omit-frame-pointer support on i386
    
    Commit 6b1df8b27f fixed the -OS build issue on i386 (BZ#20729) by
    expliciting disabling frame pointer (-fomit-frame-pointer) on the
    faulty objects.  Although it does fix the issue, it is a subpar
    workaround that adds complexity in build process (a rule for each
    object to add the required compiler option and pontentially more
    rules for objects that call {INLINE,INTERNAL}_SYSCALL) and does not
    allow the implementations to get all the possible debug/calltrack
    information possible (used mainly in debuggers and performance
    measurement tools).
    
    This patch fixes it by adding an explicit configure check to see
    if -fno-omit-frame-pointer is set and to act accordingly (set or
    not OPTIMIZE_FOR_GCC_5).  The make rules is simplified and only
    one is required: to add libc-do-syscall on loader due mmap
    (which will be empty anyway for default build with
    -fomit-frame-pointer).
    
    Checked on i386-linux-gnu with GCC 6.2.1 with CFLAGS sets as
    '-Os', '-O2 -fno-omit-frame-pointer', and '-O2 -fomit-frame-pointer'.
    For '-Os' the testsuite issues described by BZ#19463 and BZ#15105
    still applied.
    
    It fixes BZ #21029, although it is marked as duplicated of #20729
    (I reopened to track this cleanup).
    
    	[BZ #21029]
    	* config.h.in [CAN_USE_REGISTER_ASM_EBP]: New define.
    	* sysdeps/unix/sysv/linux/i386/Makefile
    	[$(subdir) = elf] (sysdep-dl-routines): Add libc-do-syscall.
    	(uses-6-syscall-arguments): Remove.
    	[$(subdir) = misc] (CFLAGS-epoll_pwait.o): Likewise.
    	[$(subdir) = misc] (CFLAGS-epoll_pwait.os): Likewise.
    	[$(subdir) = misc] (CFLAGS-mmap.o): Likewise.
    	[$(subdir) = misc] (CFLAGS-mmap.os): Likewise.
    	[$(subdir) = misc] (CFLAGS-mmap64.o): Likewise.
    	[$(subdir) = misc] (CFLAGS-mmap64.os): Likewise.
    	[$(subdir) = misc] (CFLAGS-pselect.o): Likewise.
    	[$(subdir) = misc] (cflags-pselect.o): Likewise.
    	[$(subdir) = misc] (cflags-pselect.os): Likewise.
    	[$(subdir) = misc] (cflags-rtld-mmap.os): Likewise.
    	[$(subdir) = sysvipc] (cflags-semtimedop.o): Likewise.
    	[$(subdir) = sysvipc] (cflags-semtimedop.os): Likewise.
    	[$(subdir) = io] (CFLAGS-posix_fadvise64.o): Likewise.
    	[$(subdir) = io] (CFLAGS-posix_fadvise64.os): Likewise.
    	[$(subdir) = io] (CFLAGS-posix_fallocate.o): Likewise.
    	[$(subdir) = io] (CFLAGS-posix_fallocate.os): Likewise.
    	[$(subdir) = io] (CFLAGS-posix_fallocate64.o): Likewise.
    	[$(subdir) = io] (CFLAGS-posix_fallocate64.os): Likewise.
    	[$(subdir) = io] (CFLAGS-sync_file_range.o): Likewise.
    	[$(subdir) = io] (CFLAGS-sync_file_range.os): Likewise.
    	[$(subdir) = io] (CFLAGS-fallocate.o): Likewise.
    	[$(subdir) = io] (CFLAGS-fallocate.os): Likewise.
    	[$(subdir) = io] (CFLAGS-fallocate64.o): Likewise.
    	[$(subdir) = io] (CFLAGS-fallocate64.os): Likewise.
    	[$(subdir) = nptl] (CFLAGS-pthread_rwlock_timedrdlock.o):
    	Likewise.
    	[$(subdir) = nptl] (CFLAGS-pthread_rwlock_timedrdlock.os):
    	Likewise.
    	[$(subdir) = nptl] (CFLAGS-pthread_rwlock_timedrwlock.o):
    	Likewise.
    	[$(subdir) = nptl] (CFLAGS-pthread_rwlock_timedrwlock.os):
    	Likewise.
    	[$(subdir) = nptl] (CFLAGS-sem_wait.o): Likewise.
    	[$(subdir) = nptl] (CFLAGS-sem_wait.os): Likewise.
    	[$(subdir) = nptl] (CFLAGS-sem_timedwait.o): Likewise.
    	[$(subdir) = nptl] (CFLAGS-sem_timedwait.os): Likewise.
    	* sysdeps/unix/sysv/linux/i386/configure.ac: Add check if compiler allows
    	ebp on inline assembly.
    	* sysdeps/unix/sysv/linux/i386/configure: Regenerate.
    	* sysdeps/unix/sysv/linux/i386/sysdep.h (OPTIMIZE_FOR_GCC_5):
    	Set if CAN_USE_REGISTER_ASM_EBP is set.
    	(check_consistency): Likewise.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                                 |   49 +++++++++++++++++++++++++++++
 config.h.in                               |    4 ++
 sysdeps/unix/sysv/linux/i386/Makefile     |   39 +----------------------
 sysdeps/unix/sysv/linux/i386/configure    |   39 +++++++++++++++++++++++
 sysdeps/unix/sysv/linux/i386/configure.ac |   17 ++++++++++
 sysdeps/unix/sysv/linux/i386/sysdep.h     |    6 ++--
 6 files changed, 113 insertions(+), 41 deletions(-)
Comment 9 Adhemerval Zanella 2017-02-17 20:08:16 UTC
Fixed by 3b33d6ed6096c1d.
Comment 10 Jan Ziak (http://atom-symbol.net) 2017-02-17 22:41:03 UTC
(In reply to Adhemerval Zanella from comment #9)
> Fixed by 3b33d6ed6096c1d.

Thanks.
Comment 11 Jan Ziak (http://atom-symbol.net) 2017-02-18 11:33:19 UTC
(In reply to Adhemerval Zanella from comment #9)
> Fixed by 3b33d6ed6096c1d.

The Changelog contains the text "2017-01-17  Adhemerval Zanella ...".

Shouldn't the correct date be 2017-02-17?
Comment 12 Adhemerval Zanella 2017-02-18 14:32:06 UTC
I will fix it.