| Message ID | 20180222231442.29507-3-labbott@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Laura, Thanks for the cooperation! On 23.02.2018 02:14, Laura Abbott wrote: > +#if BUILDING_GCC_VERSION >= 8000 > +bool check_frame_size() > +{ > + return maybe_ge(get_frame_size(), track_frame_size); After looking through this guide https://gcc.gnu.org/onlinedocs//gccint/Guidelines-for-using-poly_005fint.html#Guidelines-for-using-poly_005fint it seems to me that we should better use something like that: poly_int64 frame_size = get_frame_size(); if (frame_size.to_constant() >= track_frame_size) return 0; May I ask for your opinion? > +} > +#else > +bool check_frame_size() > +{ > + return get_frame_size() >= track_frame_size; > +} > +#endif > /* > * Work with the RTL representation of the code. > * Remove the unneeded track_stack() calls from the functions which don't > @@ -215,7 +237,7 @@ static unsigned int stackleak_final_execute(void) > if (cfun->calls_alloca) > return 0; > > - if (get_frame_size() >= track_frame_size) > + if (check_frame_size()) > return 0; > Best regards, Alexander
On 02/24/2018 06:04 AM, Alexander Popov wrote: > Hello Laura, > > Thanks for the cooperation! > > On 23.02.2018 02:14, Laura Abbott wrote: >> +#if BUILDING_GCC_VERSION >= 8000 >> +bool check_frame_size() >> +{ >> + return maybe_ge(get_frame_size(), track_frame_size); > > After looking through this guide > https://gcc.gnu.org/onlinedocs//gccint/Guidelines-for-using-poly_005fint.html#Guidelines-for-using-poly_005fint > it seems to me that we should better use something like that: > > poly_int64 frame_size = get_frame_size(); > > if (frame_size.to_constant() >= track_frame_size) > return 0; > > May I ask for your opinion? > I was a bit wary of using to_constant() because I wasn't 100% sure I could make the assertion that it could actually be a constant so using maybe_ge seemed like the conservative approach. This also getting into compiler internals so it's possible I'm misunderstanding how poly_ints are supposed to work. Thanks, Laura >> +} >> +#else >> +bool check_frame_size() >> +{ >> + return get_frame_size() >= track_frame_size; >> +} >> +#endif >> /* >> * Work with the RTL representation of the code. >> * Remove the unneeded track_stack() calls from the functions which don't >> @@ -215,7 +237,7 @@ static unsigned int stackleak_final_execute(void) >> if (cfun->calls_alloca) >> return 0; >> >> - if (get_frame_size() >= track_frame_size) >> + if (check_frame_size()) >> return 0; >> > > Best regards, > Alexander >
Laura Abbott <labbott@redhat.com> writes: > On 02/24/2018 06:04 AM, Alexander Popov wrote: >> Hello Laura, >> >> Thanks for the cooperation! >> >> On 23.02.2018 02:14, Laura Abbott wrote: >>> +#if BUILDING_GCC_VERSION >= 8000 >>> +bool check_frame_size() >>> +{ >>> + return maybe_ge(get_frame_size(), track_frame_size); >> >> After looking through this guide >> https://gcc.gnu.org/onlinedocs//gccint/Guidelines-for-using-poly_005fint.html#Guidelines-for-using-poly_005fint >> it seems to me that we should better use something like that: >> >> poly_int64 frame_size = get_frame_size(); >> >> if (frame_size.to_constant() >= track_frame_size) >> return 0; >> >> May I ask for your opinion? >> > > I was a bit wary of using to_constant() because I wasn't 100% sure > I could make the assertion that it could actually be a constant so using Yeah, the assert would fire for SVE functions that spill SVE registers to the stack. > maybe_ge seemed like the conservative approach. This also getting into > compiler internals so it's possible I'm misunderstanding how poly_ints > are supposed to work. The maybe_ge patch looks good to me FWIW. Thanks, Richard
On 27.02.2018 13:30, Richard Sandiford wrote: > Laura Abbott <labbott@redhat.com> writes: >> On 02/24/2018 06:04 AM, Alexander Popov wrote: >>> Hello Laura, >>> >>> Thanks for the cooperation! >>> >>> On 23.02.2018 02:14, Laura Abbott wrote: >>>> +#if BUILDING_GCC_VERSION >= 8000 >>>> +bool check_frame_size() >>>> +{ >>>> + return maybe_ge(get_frame_size(), track_frame_size); >>> >>> After looking through this guide >>> https://gcc.gnu.org/onlinedocs//gccint/Guidelines-for-using-poly_005fint.html#Guidelines-for-using-poly_005fint >>> it seems to me that we should better use something like that: >>> >>> poly_int64 frame_size = get_frame_size(); >>> >>> if (frame_size.to_constant() >= track_frame_size) >>> return 0; >>> >>> May I ask for your opinion? >>> >> >> I was a bit wary of using to_constant() because I wasn't 100% sure >> I could make the assertion that it could actually be a constant so using > > Yeah, the assert would fire for SVE functions that spill SVE registers > to the stack. > >> maybe_ge seemed like the conservative approach. This also getting into >> compiler internals so it's possible I'm misunderstanding how poly_ints >> are supposed to work. > > The maybe_ge patch looks good to me FWIW. Thanks, Laura and Richard! I see the rationale, I will use maybe_ge() in the next version of the patch. Best regards, Alexander
diff --git a/scripts/gcc-plugins/stackleak_plugin.c b/scripts/gcc-plugins/stackleak_plugin.c index 6fc991c98d8b..4ea2fdb987e6 100644 --- a/scripts/gcc-plugins/stackleak_plugin.c +++ b/scripts/gcc-plugins/stackleak_plugin.c @@ -69,8 +69,13 @@ static void stackleak_check_alloca(gimple_stmt_iterator *gsi) node = cgraph_get_create_node(check_function_decl); gcc_assert(node); frequency = compute_call_stmt_bb_frequency(current_function_decl, bb); +#if BUILDING_GCC_VERSION >= 8000 + cgraph_create_edge(cgraph_get_node(current_function_decl), node, + check_alloca, bb->count, bb->loop_depth); +#else cgraph_create_edge(cgraph_get_node(current_function_decl), node, check_alloca, bb->count, frequency, bb->loop_depth); +#endif } static void stackleak_add_instrumentation(gimple_stmt_iterator *gsi, bool after) @@ -94,8 +99,13 @@ static void stackleak_add_instrumentation(gimple_stmt_iterator *gsi, bool after) node = cgraph_get_create_node(track_function_decl); gcc_assert(node); frequency = compute_call_stmt_bb_frequency(current_function_decl, bb); +#if BUILDING_GCC_VERSION >= 8000 + cgraph_create_edge(cgraph_get_node(current_function_decl), node, + track_stack, bb->count, bb->loop_depth); +#else cgraph_create_edge(cgraph_get_node(current_function_decl), node, track_stack, bb->count, frequency, bb->loop_depth); +#endif } static bool is_alloca(gimple stmt) @@ -203,6 +213,18 @@ static unsigned int stackleak_tree_instrument_execute(void) return 0; } + +#if BUILDING_GCC_VERSION >= 8000 +bool check_frame_size() +{ + return maybe_ge(get_frame_size(), track_frame_size); +} +#else +bool check_frame_size() +{ + return get_frame_size() >= track_frame_size; +} +#endif /* * Work with the RTL representation of the code. * Remove the unneeded track_stack() calls from the functions which don't @@ -215,7 +237,7 @@ static unsigned int stackleak_final_execute(void) if (cfun->calls_alloca) return 0; - if (get_frame_size() >= track_frame_size) + if (check_frame_size()) return 0; /*
- Use the new cgraph_create_edge API - Account for the change in type for get_frame_size Signed-off-by: Laura Abbott <labbott@redhat.com> --- scripts/gcc-plugins/stackleak_plugin.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)