Re: [PATCH] Add sanopt support for UBSAN_PTR.

[Martin Sebor <msebor at gmail dot com>]

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.

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.

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


+/* 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.

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

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