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 <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
