Calling 'abort' on bounds violations in libmpx

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.

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

Calling 'abort' on bounds violations in libmpx

Commit Message

Comments

Patch