On 10/05/2017 07:06 PM, Martin Sebor wrote:
> On 10/04/2017 03:05 AM, Martin Liška wrote:
>> Hello.
>>
>> Following patch adds support for optimizing out unnecessary UBSAN_PTR checks.
>> It handles separately positive and negative offsets, zero offset is ignored.
>> Apart from that, we utilize get_inner_reference for local and global
>> variables,
>> that also helps to reduce some.
>>
>> Some numbers:
>>
>> 1) postgres:
>>
>> bloaty /tmp/after2 -- /tmp/before2
>> VM SIZE FILE SIZE
>> ++++++++++++++ GROWING ++++++++++++++
>> [ = ] 0 .debug_abbrev +1.84Ki +0.3%
>>
>> -------------- SHRINKING --------------
>> -30.3% -3.98Mi .text -3.98Mi -30.3%
>> [ = ] 0 .debug_info -3.69Mi -17.2%
>> [ = ] 0 .debug_loc -2.02Mi -13.4%
>> -43.1% -1.37Mi .data -1.37Mi -43.1%
>> [ = ] 0 .debug_ranges -390Ki -14.3%
>> [ = ] 0 .debug_line -295Ki -11.6%
>> -4.0% -26.3Ki .eh_frame -26.3Ki -4.0%
>> [ = ] 0 [Unmapped] -1.61Ki -62.3%
>> [ = ] 0 .strtab -1.15Ki -0.4%
>> [ = ] 0 .symtab -1.08Ki -0.3%
>> -0.4% -368 .eh_frame_hdr -368 -0.4%
>> [ = ] 0 .debug_aranges -256 -0.7%
>> [DEL] -16 [None] 0 [ = ]
>>
>> -28.0% -5.37Mi TOTAL -11.8Mi -18.8%
>
> This looks like an impressive improvement! FWIW, I've been
> meaning to look into similar opportunities mentioned in bug
> 79265.
Hi. Thank you very much for feedback. If you want I can help with the PR?
>
> Would making use of get_range_info() make sense here to detect
> and eliminate even more cases?
>
> Just a few minor mostly stylistic suggestions.
>
> +/* Traits class for tree triplet hash maps below. */
> +
> +struct sanopt_tree_couple_hash : typed_noop_remove <sanopt_tree_couple>
> +{
> ...
> + static inline bool
> + equal (const sanopt_tree_couple &ref1, const sanopt_tree_couple &ref2)
>
> Member functions defined within the body of the class are implicitly
> inline so while not wrong, there is no need to declare them inline
> explicitly.
Done that in v2.
>
> Also, since mark_deleted uses reinterpret_cast (as suggested by
> GCC coding conventions) it seems that is_deleted should do the
> same for consistency. Alternatively, if there isn't enough
> interest/consensus to follow this guideline perhaps it should
> be removed from the GCC coding conventions. (Very few GCC code
> seems to use reinterpret_cast.)
Likewise.
>
>
> +/* Return true when pointer PTR for a given OFFSET is already sanitized
> + in a given sanitization context CTX. */
>
> Shouldn't the comment read CUR_OFFSET? I ask because the function
> also declares a local variable OFFSET.
Yes, should be fixed.
>
> +static bool
> +has_dominating_ubsan_ptr_check (sanopt_ctx *ctx, tree ptr, tree cur_offset)
> +{
> ...
> + /* We already have recorded a UBSAN_PTR check for this pointer. Perhaps we
> + can drop this one. But only if this check doesn't specify larger
> offset.
> + */
> + tree offset = gimple_call_arg (g, 1);
>
> Martin
>
> PS It seems to me that the test could be enabled for all targets
> where UBSan is supported by making use of SIZE_MAX to compute
> the values of the constants instead of hardwiring LP64 values.
> I noticed the test doesn't exercise member arrays. Are those
> not handled by the patch?
I decided to use __PTRDIF__MAX__ as I need signed type. Is it ok?
Martin
>
>> Left checks:
>> 261039
>> Optimized out:
>> 85643
>>
>> 2) tramp3d:
>>
>> bloaty after -- before
>> VM SIZE FILE SIZE
>> ++++++++++++++ GROWING ++++++++++++++
>> +167% +30 [Unmapped] +1.01Ki +39%
>>
>> -------------- SHRINKING --------------
>> -58.5% -2.52Mi .text -2.52Mi -58.5%
>> -64.2% -574Ki .data -574Ki -64.2%
>> -5.7% -4.27Ki .eh_frame -4.27Ki -5.7%
>> -6.4% -1.06Ki .gcc_except_table -1.06Ki -6.4%
>> -7.2% -192 .bss 0 [ = ]
>> -0.1% -32 .rodata -32 -0.1%
>> [ = ] 0 .strtab -29 -0.0%
>> -1.1% -24 .dynsym -24 -1.1%
>> -1.5% -24 .rela.plt -24 -1.5%
>> [ = ] 0 .symtab -24 -0.1%
>> -0.6% -16 .dynstr -16 -0.6%
>> -1.5% -16 .plt -16 -1.5%
>> -1.4% -8 .got.plt -8 -1.4%
>> -0.6% -4 .hash -4 -0.6%
>> -1.1% -2 .gnu.version -2 -1.1%
>>
>> -58.0% -3.09Mi TOTAL -3.08Mi -55.0%
>>
>> Left checks:
>> 31131
>> Optimized out:
>> 36752
>>
>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>
>> Ready to be installed?
>> Martin
>>
>> gcc/ChangeLog:
>>
>> 2017-10-04 Martin Liska <mli...@suse.cz>
>>
>> * sanopt.c (struct sanopt_tree_couple): New struct.
>> (struct sanopt_tree_couple_hash): Likewise.
>> (struct sanopt_ctx): Add ptr_check_map.
>> (has_dominating_ubsan_ptr_check): New function.
>> (record_ubsan_ptr_check_stmt): Likewise.
>> (maybe_optimize_ubsan_ptr_ifn): Likewise.
>> (sanopt_optimize_walker): Handle IFN_UBSAN_PTR. Dump info
>> inline and newly print stmts that are left in code stream.
>> (pass_sanopt::execute): Handle also SANITIZE_POINTER_OVERFLOW.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2017-10-04 Martin Liska <mli...@suse.cz>
>>
>> * c-c++-common/ubsan/ptr-overflow-sanitization-1.c: New test.
>> ---
>> gcc/sanopt.c | 212
>> ++++++++++++++++++++-
>> .../ubsan/ptr-overflow-sanitization-1.c | 38 ++++
>> 2 files changed, 244 insertions(+), 6 deletions(-)
>> create mode 100644
>> gcc/testsuite/c-c++-common/ubsan/ptr-overflow-sanitization-1.c
>>
>>
>
