From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 99925 invoked by alias); 26 Dec 2016 15:15:55 -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 99907 invoked by uid 89); 26 Dec 2016 15:15:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=aivchenk@gmail.com, aivchenkgmailcom, 487,6, H*MI:sk:CACysSh X-HELO: mail-qk0-f175.google.com Received: from mail-qk0-f175.google.com (HELO mail-qk0-f175.google.com) (209.85.220.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 26 Dec 2016 15:15:44 +0000 Received: by mail-qk0-f175.google.com with SMTP id n21so194238256qka.3 for ; Mon, 26 Dec 2016 07:15:44 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=5BTXeUgk4eZW3U0Jj9ZUkqdy+aJRCVYyp/gmxxYDxDo=; b=Ni3rUIqsQlLFD/jwZr7Hv0I/XFicivv0I/s4mOZn4tjB14VykDXfal12hZe7Obo5B+ B1yhn88HAX4z3AShSIKB+SYWKXnYefs40xW/3s3nlks1sYeifPAHWO19WZ20e36AwJnG Phaas35sLettcslCl3T0Tn7TC9mZ3cUxc08TVI5kfQpCS9+h8Uh1DzH2wClDmROyZNY5 +QSh83miIUFLHkAAH6bnJ8TWNTrjvd1cCdCEKDNQyOid3xVNzVq1ck8wILevL/VNPP7L vh1ehJOm21z1XhUmMSoLLdHzDxy3WkJFMdu/qnXyd8aZOVdOGK5DkeDHtOMtm8kwJmOA eVVA== X-Gm-Message-State: AIkVDXI80ayoBYKpEsfSbfXDicmfmoPDojUreUQzXAcHRDeFEL4rEpZXwzmpjwCS6vOWmQ2pZ54JoaMjTQosxg== X-Received: by 10.233.221.130 with SMTP id r124mr26560081qkf.183.1482765342325; Mon, 26 Dec 2016 07:15:42 -0800 (PST) MIME-Version: 1.0 Received: by 10.200.40.132 with HTTP; Mon, 26 Dec 2016 07:15:01 -0800 (PST) In-Reply-To: References: From: Alexander Ivchenko Date: Mon, 26 Dec 2016 18:14: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/msg01908.txt.bz2 Submitted as r243928. Thank you 2016-12-08 20:22 GMT+03:00 Ilya Enkovich : > 2016-12-08 12:21 GMT+03:00 Alexander Ivchenko : >> I've tested the patch on MPX HW, no new regressions. Attached the >> final version below, would that be ok to submit? > > The patch is OK. > > Ilya > >> >> >> 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