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
