Re: [PATCH 2/3] Provide ucontext to signal handlers

[Jon TURNEY <jon.turney@dronecode.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 2749 bytes --]

On 04/04/2015 09:40, Corinna Vinschen wrote:
> On Apr  3 23:09, Jon TURNEY wrote:
>> On 01/04/2015 15:22, Corinna Vinschen wrote:
>>> On Apr  1 14:19, Jon TURNEY wrote:
>>>> Add ucontext.h header, defining ucontext_t and mcontext_t types.
>>>>
>>>> Provide sigaction sighandlers with a ucontext_t parameter, containing stack and
>>>> context information.
>>>>
>>>> 	* include/sys/ucontext.h : New header.
>>>> 	* include/ucontext.h : Ditto.
>>>> 	* exceptions.cc (call_signal_handler): Provide ucontext_t
>>>> 	parameter to signal handler function.
>>>
>>> Patch is ok with a single change:  Please add a "FIXME?" comment to:
>>>
>>>    else
>>>      RtlCaptureContext();
>>>
>>> On second thought, calling RtlCaptureContext here is probably wrong.
>>
>> Wrong and also dangerous.
>>
>> This causes random crashes on x86.
>>
>> It seems that RtlCaptureContext requires the framepointer of the calling
>> function in ebp, which it uses to report the rip and rsp of it's caller.
>>
>> It also seems that gcc can decide to optimize the setting of the
>> framepointer away, irrespective of the fact that -fomit-frame-pointer is not
>> used when building exceptions.cc
>>
>> If _cygtls::call_signal_handler() happens to get called with ebp pointing to
>> an invalid memory address, as seems to happen occasionally, we will fault in
>> RtlCaptureContext.  (in all cases, the eip and ebp in the returned context
>> are incorrect)
>>
>> I wrote the attached patch, which fakes a callframe for RtlCaptureContext to
>> avoid these possible crashes, but this needs more work to correctly report
>> eip and ebp
>
> Maybe it's simpler than that?  Looking into the GCC info pages, I found
> this:
>
>       Starting with GCC version 4.6, the default setting (when not
>       optimizing for size) for 32-bit GNU/Linux x86 and 32-bit Darwin x86
>       targets has been changed to '-fomit-frame-pointer'.  The default
>       can be reverted to '-fno-omit-frame-pointer' by configuring GCC
>       with the '--enable-frame-pointer' configure option.
>
>       Enabled at levels '-O', '-O2', '-O3', '-Os'.
>       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Very good.  I hadn't noticed that sentence and was just looking at the 
"-O also turns on -fomit-frame-pointer on machines where doing so does 
not interfere with debugging. "

> So it seems adding -fomit-frame-pointer file by file in Makefile.in
> (when building with -O2) is moot and only has an effect when building
> unoptimized, otherwise all files are built with -fomit-frame-pointer
> anyway.
>
> So, what if we drop all the -fomit-frame-pointer from Makefile.in and
> add an
>
>    exceptions_CFLAGS:=-fno-omit-frame-pointer
>
> Does that help?

Yes, that seems to do the trick.  Patch attached.


[-- Attachment #2: 0001-Compile-exceptions.cc-with-fno-omit-frame-pointer-on.patch --]
[-- Type: text/plain, Size: 4250 bytes --]

From cd8532832c92d10c1a3c1fb9ec705df0ea003e28 Mon Sep 17 00:00:00 2001
From: Jon TURNEY <jon.turney@dronecode.org.uk>
Date: Sat, 4 Apr 2015 16:18:56 +0100
Subject: [PATCH] Compile exceptions.cc with -fno-omit-frame-pointer on x86

Selectively using -fomit-frame-pointer when -O is used doesn't make sense
anymore, apparently since gcc 4.6, -O implies -fomit-frame-pointer.

exceptions.cc must be compiled with -fno-omit-frame-pointer on x86, as it uses
RtlCaptureContext, which requires a frame pointer.

	* Makefile.in : Remove setting -fomit-frame-pointer for compiling
	various files, it is already the default.  Set
	-fno-omit-frame-pointer for exceptions.cc on x86.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 winsup/cygwin/ChangeLog   |  6 +++++
 winsup/cygwin/Makefile.in | 57 ++++-------------------------------------------
 2 files changed, 10 insertions(+), 53 deletions(-)

diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog
index 5ada35d..a8f4e00 100644
--- a/winsup/cygwin/ChangeLog
+++ b/winsup/cygwin/ChangeLog
@@ -1,3 +1,9 @@
+2015-04-04  Jon TURNEY  <jon.turney@dronecode.org.uk>
+
+	* Makefile.in : Remove setting -fomit-frame-pointer for compiling
+	various files, it is already the default.  Set
+	-fno-omit-frame-pointer for exceptions.cc on x86.
+
 2015-04-03  Takashi Yano  <takashi.yano@nifty.ne.jp>
 
 	* fhandler_tty.cc (fhandler_pty_slave::read): Change calculation of
diff --git a/winsup/cygwin/Makefile.in b/winsup/cygwin/Makefile.in
index 9b82f0a..af9faf6 100644
--- a/winsup/cygwin/Makefile.in
+++ b/winsup/cygwin/Makefile.in
@@ -449,59 +449,10 @@ INSTOBJS:=automode.o binmode.o textmode.o textreadmode.o
 TARGET_LIBS:=$(LIB_NAME) $(CYGWIN_START) $(GMON_START) $(LIBGMON_A) $(SUBLIBS) $(INSTOBJS) $(EXTRALIBS)
 
 ifneq "${filter -O%,$(CFLAGS)}" ""
-cygheap_CFLAGS:=-fomit-frame-pointer
-cygthread_CFLAGS:=-fomit-frame-pointer
-cygtls_CFLAGS:=-fomit-frame-pointer
-cygwait_CFLAGS=-fomit-frame-pointer
-delqueue_CFLAGS:=-fomit-frame-pointer
-devices_CFLAGS:=-fomit-frame-pointer
-dir_CFLAGS:=-fomit-frame-pointer
-dlfcn_CFLAGS:=-fomit-frame-pointer
-dll_init_CFLAGS:=-fomit-frame-pointer
-dtable_CFLAGS:=-fomit-frame-pointer -fcheck-new
-fcntl_CFLAGS:=-fomit-frame-pointer
-fenv_CFLAGS:=-fomit-frame-pointer
-fhandler_CFLAGS:=-fomit-frame-pointer
-fhandler_clipboard_CFLAGS:=-fomit-frame-pointer
-fhandler_console_CFLAGS:=-fomit-frame-pointer
-fhandler_disk_file_CFLAGS:=-fomit-frame-pointer
-fhandler_dsp_CFLAGS:=-fomit-frame-pointer
-fhandler_floppy_CFLAGS:=-fomit-frame-pointer
-fhandler_netdrive_CFLAGS:=-fomit-frame-pointer
-fhandler_proc_CFLAGS:=-fomit-frame-pointer
-fhandler_process_CFLAGS:=-fomit-frame-pointer
-fhandler_random_CFLAGS:=-fomit-frame-pointer
-fhandler_raw_CFLAGS:=-fomit-frame-pointer
-fhandler_registry_CFLAGS:=-fomit-frame-pointer
-fhandler_serial_CFLAGS:=-fomit-frame-pointer
-fhandler_socket_CFLAGS:=-fomit-frame-pointer
-fhandler_syslog_CFLAGS:=-fomit-frame-pointer
-fhandler_tape_CFLAGS:=-fomit-frame-pointer
-fhandler_termios_CFLAGS:=-fomit-frame-pointer
-fhandler_tty_CFLAGS:=-fomit-frame-pointer
-fhandler_virtual_CFLAGS:=-fomit-frame-pointer
-fhandler_windows_CFLAGS:=-fomit-frame-pointer
-fhandler_zero_CFLAGS:=-fomit-frame-pointer
-flock_CFLAGS:=-fomit-frame-pointer
-grp_CFLAGS:=-fomit-frame-pointer
-libstdcxx_wrapper_CFLAGS:=-fomit-frame-pointer
-localtime_CFLAGS:=-fwrapv
-malloc_CFLAGS:=-fomit-frame-pointer -O3
-malloc_wrapper_CFLAGS:=-fomit-frame-pointer
-miscfuncs_CFLAGS:=-fomit-frame-pointer
-net_CFLAGS:=-fomit-frame-pointer
-passwd_CFLAGS:=-fomit-frame-pointer
-path_CFLAGS=-fomit-frame-pointer
-regcomp_CFLAGS=-fomit-frame-pointer
-regerror_CFLAGS=-fomit-frame-pointer
-regexec_CFLAGS=-fomit-frame-pointer
-regfree_CFLAGS=-fomit-frame-pointer
-shared_CFLAGS:=-fomit-frame-pointer
-sync_CFLAGS:=-fomit-frame-pointer -O3
-smallprint_CFLAGS:=-fomit-frame-pointer
-syscalls_CFLAGS:=-fomit-frame-pointer
-sysconf_CFLAGS:=-fomit-frame-pointer
-uinfo_CFLAGS:=-fomit-frame-pointer
+ifeq ($(target_cpu),i686)
+# on x86, exceptions.cc must be compiled with a frame-pointer as it uses RtlCaptureContext()
+exceptions_CFLAGS:=-fno-omit-frame-pointer
+endif
 endif
 
 fhandler_proc_CFLAGS+=-DUSERNAME="\"$(USER)\"" -DHOSTNAME="\"$(HOSTNAME)\""
-- 
2.1.4