Submitted as r243928.
Thank you
2016-12-08 20:22 GMT+03:00 Ilya Enkovich <enkovich....@gmail.com>:
> 2016-12-08 12:21 GMT+03:00 Alexander Ivchenko <aivch...@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.ivche...@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....@gmail.com>:
>>> 2016-12-01 17:10 GMT+03:00 Alexander Ivchenko <aivch...@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....@gmail.com>:
>>>>> 2016-11-29 17:43 GMT+03:00 Alexander Ivchenko <aivch...@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.ivche...@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
