Message ID | CACysShgKwPchSFgo-L-Q2xO_Js1RZ_UE_NQ4scsZdjmnh+ctPQ@mail.gmail.com |
---|---|
State | New |
Headers | show |
2016-12-08 12:21 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>: > 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 <alexander.ivchenko@intel.com> > > * 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 <enkovich.gnu@gmail.com>: >> 2016-12-01 17:10 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>: >>> 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 <enkovich.gnu@gmail.com>: >>>> 2016-11-29 17:43 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>: >>>>> 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 <alexander.ivchenko@intel.com> >>>>> >>>>> * 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
Submitted as r243928. Thank you 2016-12-08 20:22 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: > 2016-12-08 12:21 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>: >> 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 <alexander.ivchenko@intel.com> >> >> * 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 <enkovich.gnu@gmail.com>: >>> 2016-12-01 17:10 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>: >>>> 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 <enkovich.gnu@gmail.com>: >>>>> 2016-11-29 17:43 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>: >>>>>> 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 <alexander.ivchenko@intel.com> >>>>>> >>>>>> * 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
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 (); } }