* Calling 'abort' on bounds violations in libmpx
@ 2016-11-29 14:44 Alexander Ivchenko
2016-11-29 17:22 ` Ilya Enkovich
0 siblings, 1 reply; 8+ messages in thread
From: Alexander Ivchenko @ 2016-11-29 14:44 UTC (permalink / raw)
To: Ilya Enkovich, GCC Patches
Hi,
Attached patch is addressing PR67520. Would that approach work for the
problem? Should I also change the version of the library?
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Calling 'abort' on bounds violations in libmpx
2016-11-29 14:44 Calling 'abort' on bounds violations in libmpx Alexander Ivchenko
@ 2016-11-29 17:22 ` Ilya Enkovich
2016-12-01 14:11 ` Alexander Ivchenko
0 siblings, 1 reply; 8+ messages in thread
From: Ilya Enkovich @ 2016-11-29 17:22 UTC (permalink / raw)
To: Alexander Ivchenko; +Cc: GCC Patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Calling 'abort' on bounds violations in libmpx
2016-11-29 17:22 ` Ilya Enkovich
@ 2016-12-01 14:11 ` Alexander Ivchenko
[not found] ` <CAMbmDYa_EmsNOpjcf=7+qqUCbAgORHTD1vpio_1e43yUiW1SUQ@mail.gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Alexander Ivchenko @ 2016-12-01 14:11 UTC (permalink / raw)
To: Ilya Enkovich; +Cc: GCC Patches
Should changing minor version of the library be enough?
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?
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Calling 'abort' on bounds violations in libmpx
[not found] ` <CAMbmDYa_EmsNOpjcf=7+qqUCbAgORHTD1vpio_1e43yUiW1SUQ@mail.gmail.com>
@ 2016-12-08 9:22 ` Alexander Ivchenko
2016-12-08 17:22 ` Ilya Enkovich
0 siblings, 1 reply; 8+ messages in thread
From: Alexander Ivchenko @ 2016-12-08 9:22 UTC (permalink / raw)
To: Ilya Enkovich; +Cc: GCC Patches
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.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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Calling 'abort' on bounds violations in libmpx
2016-12-08 9:22 ` Alexander Ivchenko
@ 2016-12-08 17:22 ` Ilya Enkovich
2016-12-26 18:14 ` Alexander Ivchenko
0 siblings, 1 reply; 8+ messages in thread
From: Ilya Enkovich @ 2016-12-08 17:22 UTC (permalink / raw)
To: Alexander Ivchenko; +Cc: GCC Patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Calling 'abort' on bounds violations in libmpx
2016-12-08 17:22 ` Ilya Enkovich
@ 2016-12-26 18:14 ` Alexander Ivchenko
2017-01-30 10:19 ` [PATCH] Avoid implicitly declared __mpxrt_stop " Jakub Jelinek
0 siblings, 1 reply; 8+ messages in thread
From: Alexander Ivchenko @ 2016-12-26 18:14 UTC (permalink / raw)
To: Ilya Enkovich; +Cc: GCC Patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] Avoid implicitly declared __mpxrt_stop in libmpx
2016-12-26 18:14 ` Alexander Ivchenko
@ 2017-01-30 10:19 ` Jakub Jelinek
2017-01-30 11:22 ` Alexander Ivchenko
0 siblings, 1 reply; 8+ messages in thread
From: Jakub Jelinek @ 2017-01-30 10:19 UTC (permalink / raw)
To: Alexander Ivchenko; +Cc: Ilya Enkovich, GCC Patches
Hi!
On Mon, Dec 26, 2016 at 06:15:01PM +0300, Alexander Ivchenko wrote:
> Submitted as r243928.
> >> (__mpxrt_stop): Ditto.
I've noticed:
../../../../libmpx/mpxrt/mpxrt.c:255:6: warning: implicit declaration of function ‘__mpxrt_stop’; did you mean ‘__mpxrt_mode’? [-Wimplicit-function
__mpxrt_stop ();
^~~~~~~~~~~~
__mpxrt_mode
The following patch fixes that warning, ok for trunk?
2017-01-30 Jakub Jelinek <jakub@redhat.com>
* mpxrt/mpxrt-utils.h (__mpxrt_stop): New prototype.
--- gcc/mpxrt/mpxrt-utils.h.jj 2016-12-27 15:35:06.000000000 +0100
+++ gcc/mpxrt/mpxrt-utils.h 2017-01-30 10:31:51.502825561 +0100
@@ -66,5 +66,6 @@ void __mpxrt_print (verbose_type vt, con
mpx_rt_mode_t __mpxrt_mode (void);
void __mpxrt_utils_free (void);
void __mpxrt_print_summary (uint64_t num_brs, uint64_t l1_size);
+void __mpxrt_stop (void) __attribute__ ((noreturn));
#endif /* MPXRT_UTILS_H */
Jakub
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Avoid implicitly declared __mpxrt_stop in libmpx
2017-01-30 10:19 ` [PATCH] Avoid implicitly declared __mpxrt_stop " Jakub Jelinek
@ 2017-01-30 11:22 ` Alexander Ivchenko
0 siblings, 0 replies; 8+ messages in thread
From: Alexander Ivchenko @ 2017-01-30 11:22 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Alexander Ivchenko, Ilya Enkovich, GCC Patches
Hi Jakub,
Thanks for noticing that. Of course the declaration had to be in the
header for this case
Alexander
2017-01-30 13:13 GMT+03:00 Jakub Jelinek <jakub@redhat.com>:
> Hi!
>
> On Mon, Dec 26, 2016 at 06:15:01PM +0300, Alexander Ivchenko wrote:
>> Submitted as r243928.
>> >> (__mpxrt_stop): Ditto.
>
> I've noticed:
> ../../../../libmpx/mpxrt/mpxrt.c:255:6: warning: implicit declaration of function ‘__mpxrt_stop’; did you mean ‘__mpxrt_mode’? [-Wimplicit-function
> __mpxrt_stop ();
> ^~~~~~~~~~~~
> __mpxrt_mode
>
> The following patch fixes that warning, ok for trunk?
>
> 2017-01-30 Jakub Jelinek <jakub@redhat.com>
>
> * mpxrt/mpxrt-utils.h (__mpxrt_stop): New prototype.
>
> --- gcc/mpxrt/mpxrt-utils.h.jj 2016-12-27 15:35:06.000000000 +0100
> +++ gcc/mpxrt/mpxrt-utils.h 2017-01-30 10:31:51.502825561 +0100
> @@ -66,5 +66,6 @@ void __mpxrt_print (verbose_type vt, con
> mpx_rt_mode_t __mpxrt_mode (void);
> void __mpxrt_utils_free (void);
> void __mpxrt_print_summary (uint64_t num_brs, uint64_t l1_size);
> +void __mpxrt_stop (void) __attribute__ ((noreturn));
>
> #endif /* MPXRT_UTILS_H */
>
>
> Jakub
--
--Alexander
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-01-30 11:22 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-29 14:44 Calling 'abort' on bounds violations in libmpx Alexander Ivchenko
2016-11-29 17:22 ` Ilya Enkovich
2016-12-01 14:11 ` Alexander Ivchenko
[not found] ` <CAMbmDYa_EmsNOpjcf=7+qqUCbAgORHTD1vpio_1e43yUiW1SUQ@mail.gmail.com>
2016-12-08 9:22 ` Alexander Ivchenko
2016-12-08 17:22 ` Ilya Enkovich
2016-12-26 18:14 ` Alexander Ivchenko
2017-01-30 10:19 ` [PATCH] Avoid implicitly declared __mpxrt_stop " Jakub Jelinek
2017-01-30 11:22 ` Alexander Ivchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).