From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31033 invoked by alias); 8 Dec 2016 09:22:03 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 31016 invoked by uid 89); 8 Dec 2016 09:22:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=AGE, CURRENT, 20161129, 4876 X-HELO: mail-qk0-f169.google.com Received: from mail-qk0-f169.google.com (HELO mail-qk0-f169.google.com) (209.85.220.169) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 08 Dec 2016 09:21:52 +0000 Received: by mail-qk0-f169.google.com with SMTP id n204so444014160qke.2 for ; Thu, 08 Dec 2016 01:21:52 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=NXsEDlm99sg1GSFT2ixm+/zZoTkn31VsjRLIBxgIE5s=; b=HbpEXqV4eB588l8bnXEH+fWeHu/vNkgsNmesBcgOxKIwLpyrkObbmf7TS4yxhu+3cE 61nrwIjnitB7pL1qVpA8eD/LrfHsJSeEOmUO5+jyoVOoje0cXELoPdtCqr0z38EIZxKl ibnBgkgW3KQJVuXKMvjDioPaOFxGRAdPoqCBB+dk61pxuNtqdAMsZKbLNgBPRZ09bAYY RaLYglLTMsHhWAdKPiV1BrArr60Q2jLko9oEiSW3qLg+sC/lcjSrtqUYuTdXuxyleRm/ DsUSapvDZXUXSvKRrf7pqcLfOWiuZGgR5EOtCUC/pf7s0aNu5pFcxX6QGJxgMsag51TZ jR5g== X-Gm-Message-State: AKaTC01jaXsttHFNIgNBAIvtGlYibag2BzB8uIcDYv0FWjMMx+G9K9iJPy6Hpn5xhLh2GxBXpnQQvFhL1SXMPA== X-Received: by 10.55.15.209 with SMTP id 78mr10789227qkp.269.1481188910331; Thu, 08 Dec 2016 01:21:50 -0800 (PST) MIME-Version: 1.0 Received: by 10.200.40.132 with HTTP; Thu, 8 Dec 2016 01:21:10 -0800 (PST) In-Reply-To: References: From: Alexander Ivchenko Date: Thu, 08 Dec 2016 09:22:00 -0000 Message-ID: Subject: Re: Calling 'abort' on bounds violations in libmpx To: Ilya Enkovich Cc: GCC Patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2016-12/txt/msg00631.txt.bz2 I've tested the patch on MPX HW, no new regressions. Attached the final version below, would that be ok to submit? 2016-11-29 Alexander Ivchenko * mpxrt/libtool-version: New version. * mpxrt/mpxrt-utils.c (set_mpx_rt_stop_handler): New function. (print_help): Add help for CHKP_RT_STOP_HANDLER environment variable. (__mpxrt_init_env_vars): Add initialization of stop_handler. (__mpxrt_stop_handler): New function. (__mpxrt_stop): Ditto. * mpxrt/mpxrt-utils.h (mpx_rt_stop_mode_handler_t): New enum. diff --git a/libmpx/mpxrt/libtool-version b/libmpx/mpxrt/libtool-version index 7d99255..736d763 100644 --- a/libmpx/mpxrt/libtool-version +++ b/libmpx/mpxrt/libtool-version @@ -3,4 +3,4 @@ # a separate file so that version updates don't involve re-running # automake. # CURRENT:REVISION:AGE -2:0:0 +2:1:0 diff --git a/libmpx/mpxrt/mpxrt-utils.c b/libmpx/mpxrt/mpxrt-utils.c index 057a355..63ee7c6 100644 --- a/libmpx/mpxrt/mpxrt-utils.c +++ b/libmpx/mpxrt/mpxrt-utils.c @@ -60,6 +60,9 @@ #define MPX_RT_MODE "CHKP_RT_MODE" #define MPX_RT_MODE_DEFAULT MPX_RT_COUNT #define MPX_RT_MODE_DEFAULT_STR "count" +#define MPX_RT_STOP_HANDLER "CHKP_RT_STOP_HANDLER" +#define MPX_RT_STOP_HANDLER_DEFAULT MPX_RT_STOP_HANDLER_ABORT +#define MPX_RT_STOP_HANDLER_DEFAULT_STR "abort" #define MPX_RT_HELP "CHKP_RT_HELP" #define MPX_RT_ADDPID "CHKP_RT_ADDPID" #define MPX_RT_BNDPRESERVE "CHKP_RT_BNDPRESERVE" @@ -84,6 +87,7 @@ typedef struct { static int summary; static int add_pid; static mpx_rt_mode_t mode; +static mpx_rt_stop_mode_handler_t stop_handler; static env_var_list_t env_var_list; static verbose_type verbose_val; static FILE *out; @@ -226,6 +230,23 @@ set_mpx_rt_mode (const char *env) } } +static mpx_rt_stop_mode_handler_t +set_mpx_rt_stop_handler (const char *env) +{ + if (env == 0) + return MPX_RT_STOP_HANDLER_DEFAULT; + else if (strcmp (env, "abort") == 0) + return MPX_RT_STOP_HANDLER_ABORT; + else if (strcmp (env, "exit") == 0) + return MPX_RT_STOP_HANDLER_EXIT; + { + __mpxrt_print (VERB_ERROR, "Illegal value '%s' for %s. Legal values are" + "[abort | exit]\nUsing default value %s\n", + env, MPX_RT_STOP_HANDLER, MPX_RT_STOP_HANDLER_DEFAULT); + return MPX_RT_STOP_HANDLER_DEFAULT; + } +} + static void print_help (void) { @@ -244,6 +265,11 @@ print_help (void) fprintf (out, "%s \t\t set MPX runtime behavior on #BR exception." " [stop | count]\n" "\t\t\t [default: %s]\n", MPX_RT_MODE, MPX_RT_MODE_DEFAULT_STR); + fprintf (out, "%s \t set the handler function MPX runtime will call\n" + "\t\t\t on #BR exception when %s is set to \'stop\'." + " [abort | exit]\n" + "\t\t\t [default: %s]\n", MPX_RT_STOP_HANDLER, MPX_RT_MODE, + MPX_RT_STOP_HANDLER_DEFAULT_STR); fprintf (out, "%s \t\t generate out,err file for each process.\n" "\t\t\t generated file will be MPX_RT_{OUT,ERR}_FILE.pid\n" "\t\t\t [default: no]\n", MPX_RT_ADDPID); @@ -357,6 +383,10 @@ __mpxrt_init_env_vars (int* bndpreserve) env_var_list_add (MPX_RT_MODE, env); mode = set_mpx_rt_mode (env); + env = secure_getenv (MPX_RT_STOP_HANDLER); + env_var_list_add (MPX_RT_STOP_HANDLER, env); + stop_handler = set_mpx_rt_stop_handler (env); + env = secure_getenv (MPX_RT_BNDPRESERVE); env_var_list_add (MPX_RT_BNDPRESERVE, env); validate_bndpreserve (env, bndpreserve); @@ -487,6 +517,22 @@ __mpxrt_mode (void) return mode; } +mpx_rt_mode_t +__mpxrt_stop_handler (void) +{ + return stop_handler; +} + +void __attribute__ ((noreturn)) +__mpxrt_stop (void) +{ + if (__mpxrt_stop_handler () == MPX_RT_STOP_HANDLER_ABORT) + abort (); + else if (__mpxrt_stop_handler () == MPX_RT_STOP_HANDLER_EXIT) + exit (255); + __builtin_unreachable (); +} + void __mpxrt_print_summary (uint64_t num_brs, uint64_t l1_size) { diff --git a/libmpx/mpxrt/mpxrt-utils.h b/libmpx/mpxrt/mpxrt-utils.h index d62937d..6da12cc 100644 --- a/libmpx/mpxrt/mpxrt-utils.h +++ b/libmpx/mpxrt/mpxrt-utils.h @@ -54,6 +54,11 @@ typedef enum { MPX_RT_STOP } mpx_rt_mode_t; +typedef enum { + MPX_RT_STOP_HANDLER_ABORT, + MPX_RT_STOP_HANDLER_EXIT +} mpx_rt_stop_mode_handler_t; + void __mpxrt_init_env_vars (int* bndpreserve); void __mpxrt_write_uint (verbose_type vt, uint64_t val, unsigned base); void __mpxrt_write (verbose_type vt, const char* str); diff --git a/libmpx/mpxrt/mpxrt.c b/libmpx/mpxrt/mpxrt.c index b52906b..76d11f7 100644 --- a/libmpx/mpxrt/mpxrt.c +++ b/libmpx/mpxrt/mpxrt.c @@ -252,7 +252,7 @@ handler (int sig __attribute__ ((unused)), uctxt->uc_mcontext.gregs[REG_IP_IDX] = (greg_t)get_next_inst_ip ((uint8_t *)ip); if (__mpxrt_mode () == MPX_RT_STOP) - exit (255); + __mpxrt_stop (); return; default: @@ -269,7 +269,7 @@ handler (int sig __attribute__ ((unused)), __mpxrt_write (VERB_ERROR, ", ip = 0x"); __mpxrt_write_uint (VERB_ERROR, ip, 16); __mpxrt_write (VERB_ERROR, "\n"); - exit (255); + __mpxrt_stop (); } else { @@ -278,7 +278,7 @@ handler (int sig __attribute__ ((unused)), __mpxrt_write (VERB_ERROR, "! at 0x"); __mpxrt_write_uint (VERB_ERROR, ip, 16); __mpxrt_write (VERB_ERROR, "\n"); - exit (255); + __mpxrt_stop (); } } 2016-12-01 23:32 GMT+03:00 Ilya Enkovich : > 2016-12-01 17:10 GMT+03:00 Alexander Ivchenko : >> Should changing minor version of the library be enough? > > Yes. > >> >> diff --git a/libmpx/mpxrt/libtool-version b/libmpx/mpxrt/libtool-version >> index 7d99255..736d763 100644 >> --- a/libmpx/mpxrt/libtool-version >> +++ b/libmpx/mpxrt/libtool-version >> @@ -3,4 +3,4 @@ >> # a separate file so that version updates don't involve re-running >> # automake. >> # CURRENT:REVISION:AGE >> -2:0:0 >> +2:1:0 >> >> (otherwise - no difference). >> >> I've run make check on a non-mpx-enabled machine (no new regressions) >> and manually tested newly added environment variable on the mpx >> machine. It looks like there is no explicit tests for libmpx, so I'm >> not sure what tests should I add. What do you think would be the right >> testing process here? > > Some current tests use MPX runtime now. Please check your change > in MPX runtime doesn't break them on MPX HW (on legacy HW tests > are just skipped) > > Currently all MPX tests are in i386 part of gcc.target which is not great. > Having new testsuite right in libmpx would be great but I won't require > it as a prerequisite for this patch. > > Ilya > >> >> 2016-11-29 20:22 GMT+03:00 Ilya Enkovich : >>> 2016-11-29 17:43 GMT+03:00 Alexander Ivchenko : >>>> Hi, >>>> >>>> Attached patch is addressing PR67520. Would that approach work for the >>>> problem? Should I also change the version of the library? >>> >>> Hi! >>> >>> Overall patch is OK. But you need to change version because you >>> change default behavior. How did you test it? Did you check default >>> behavior change doesn't affect existing runtime MPX tests? Can we >>> add new ones? >>> >>> Thanks, >>> Ilya >>> >>>> >>>> 2016-11-29 Alexander Ivchenko >>>> >>>> * mpxrt/mpxrt-utils.c (set_mpx_rt_stop_handler): New function. >>>> (print_help): Add help for CHKP_RT_STOP_HANDLER environment >>>> variable. >>>> (__mpxrt_init_env_vars): Add initialization of stop_handler. >>>> (__mpxrt_stop_handler): New function. >>>> (__mpxrt_stop): Ditto. >>>> * mpxrt/mpxrt-utils.h (mpx_rt_stop_mode_handler_t): New enum. >>>> >>>> >>>> >>>> diff --git a/libmpx/mpxrt/mpxrt-utils.c b/libmpx/mpxrt/mpxrt-utils.c >>>> index 057a355..63ee7c6 100644 >>>> --- a/libmpx/mpxrt/mpxrt-utils.c >>>> +++ b/libmpx/mpxrt/mpxrt-utils.c >>>> @@ -60,6 +60,9 @@ >>>> #define MPX_RT_MODE "CHKP_RT_MODE" >>>> #define MPX_RT_MODE_DEFAULT MPX_RT_COUNT >>>> #define MPX_RT_MODE_DEFAULT_STR "count" >>>> +#define MPX_RT_STOP_HANDLER "CHKP_RT_STOP_HANDLER" >>>> +#define MPX_RT_STOP_HANDLER_DEFAULT MPX_RT_STOP_HANDLER_ABORT >>>> +#define MPX_RT_STOP_HANDLER_DEFAULT_STR "abort" >>>> #define MPX_RT_HELP "CHKP_RT_HELP" >>>> #define MPX_RT_ADDPID "CHKP_RT_ADDPID" >>>> #define MPX_RT_BNDPRESERVE "CHKP_RT_BNDPRESERVE" >>>> @@ -84,6 +87,7 @@ typedef struct { >>>> static int summary; >>>> static int add_pid; >>>> static mpx_rt_mode_t mode; >>>> +static mpx_rt_stop_mode_handler_t stop_handler; >>>> static env_var_list_t env_var_list; >>>> static verbose_type verbose_val; >>>> static FILE *out; >>>> @@ -226,6 +230,23 @@ set_mpx_rt_mode (const char *env) >>>> } >>>> } >>>> >>>> +static mpx_rt_stop_mode_handler_t >>>> +set_mpx_rt_stop_handler (const char *env) >>>> +{ >>>> + if (env == 0) >>>> + return MPX_RT_STOP_HANDLER_DEFAULT; >>>> + else if (strcmp (env, "abort") == 0) >>>> + return MPX_RT_STOP_HANDLER_ABORT; >>>> + else if (strcmp (env, "exit") == 0) >>>> + return MPX_RT_STOP_HANDLER_EXIT; >>>> + { >>>> + __mpxrt_print (VERB_ERROR, "Illegal value '%s' for %s. Legal values are" >>>> + "[abort | exit]\nUsing default value %s\n", >>>> + env, MPX_RT_STOP_HANDLER, MPX_RT_STOP_HANDLER_DEFAULT); >>>> + return MPX_RT_STOP_HANDLER_DEFAULT; >>>> + } >>>> +} >>>> + >>>> static void >>>> print_help (void) >>>> { >>>> @@ -244,6 +265,11 @@ print_help (void) >>>> fprintf (out, "%s \t\t set MPX runtime behavior on #BR exception." >>>> " [stop | count]\n" >>>> "\t\t\t [default: %s]\n", MPX_RT_MODE, MPX_RT_MODE_DEFAULT_STR); >>>> + fprintf (out, "%s \t set the handler function MPX runtime will call\n" >>>> + "\t\t\t on #BR exception when %s is set to \'stop\'." >>>> + " [abort | exit]\n" >>>> + "\t\t\t [default: %s]\n", MPX_RT_STOP_HANDLER, MPX_RT_MODE, >>>> + MPX_RT_STOP_HANDLER_DEFAULT_STR); >>>> fprintf (out, "%s \t\t generate out,err file for each process.\n" >>>> "\t\t\t generated file will be MPX_RT_{OUT,ERR}_FILE.pid\n" >>>> "\t\t\t [default: no]\n", MPX_RT_ADDPID); >>>> @@ -357,6 +383,10 @@ __mpxrt_init_env_vars (int* bndpreserve) >>>> env_var_list_add (MPX_RT_MODE, env); >>>> mode = set_mpx_rt_mode (env); >>>> >>>> + env = secure_getenv (MPX_RT_STOP_HANDLER); >>>> + env_var_list_add (MPX_RT_STOP_HANDLER, env); >>>> + stop_handler = set_mpx_rt_stop_handler (env); >>>> + >>>> env = secure_getenv (MPX_RT_BNDPRESERVE); >>>> env_var_list_add (MPX_RT_BNDPRESERVE, env); >>>> validate_bndpreserve (env, bndpreserve); >>>> @@ -487,6 +517,22 @@ __mpxrt_mode (void) >>>> return mode; >>>> } >>>> >>>> +mpx_rt_mode_t >>>> +__mpxrt_stop_handler (void) >>>> +{ >>>> + return stop_handler; >>>> +} >>>> + >>>> +void __attribute__ ((noreturn)) >>>> +__mpxrt_stop (void) >>>> +{ >>>> + if (__mpxrt_stop_handler () == MPX_RT_STOP_HANDLER_ABORT) >>>> + abort (); >>>> + else if (__mpxrt_stop_handler () == MPX_RT_STOP_HANDLER_EXIT) >>>> + exit (255); >>>> + __builtin_unreachable (); >>>> +} >>>> + >>>> void >>>> __mpxrt_print_summary (uint64_t num_brs, uint64_t l1_size) >>>> { >>>> diff --git a/libmpx/mpxrt/mpxrt-utils.h b/libmpx/mpxrt/mpxrt-utils.h >>>> index d62937d..6da12cc 100644 >>>> --- a/libmpx/mpxrt/mpxrt-utils.h >>>> +++ b/libmpx/mpxrt/mpxrt-utils.h >>>> @@ -54,6 +54,11 @@ typedef enum { >>>> MPX_RT_STOP >>>> } mpx_rt_mode_t; >>>> >>>> +typedef enum { >>>> + MPX_RT_STOP_HANDLER_ABORT, >>>> + MPX_RT_STOP_HANDLER_EXIT >>>> +} mpx_rt_stop_mode_handler_t; >>>> + >>>> void __mpxrt_init_env_vars (int* bndpreserve); >>>> void __mpxrt_write_uint (verbose_type vt, uint64_t val, unsigned base); >>>> void __mpxrt_write (verbose_type vt, const char* str); >>>> diff --git a/libmpx/mpxrt/mpxrt.c b/libmpx/mpxrt/mpxrt.c >>>> index b52906b..0bc069c 100644 >>>> --- a/libmpx/mpxrt/mpxrt.c >>>> +++ b/libmpx/mpxrt/mpxrt.c >>>> @@ -252,7 +251,7 @@ handler (int sig __attribute__ ((unused)), >>>> uctxt->uc_mcontext.gregs[REG_IP_IDX] = >>>> (greg_t)get_next_inst_ip ((uint8_t *)ip); >>>> if (__mpxrt_mode () == MPX_RT_STOP) >>>> - exit (255); >>>> + __mpxrt_stop (); >>>> return; >>>> >>>> default: >>>> @@ -269,7 +268,7 @@ handler (int sig __attribute__ ((unused)), >>>> __mpxrt_write (VERB_ERROR, ", ip = 0x"); >>>> __mpxrt_write_uint (VERB_ERROR, ip, 16); >>>> __mpxrt_write (VERB_ERROR, "\n"); >>>> - exit (255); >>>> + __mpxrt_stop (); >>>> } >>>> else >>>> { >>>> @@ -278,7 +277,7 @@ handler (int sig __attribute__ ((unused)), >>>> __mpxrt_write (VERB_ERROR, "! at 0x"); >>>> __mpxrt_write_uint (VERB_ERROR, ip, 16); >>>> __mpxrt_write (VERB_ERROR, "\n"); >>>> - exit (255); >>>> + __mpxrt_stop (); >>>> } >>>> } >>>> >>>> thanks, >>>> Alexander