From patchwork Thu Nov 6 22:19:08 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marek Polacek X-Patchwork-Id: 407735 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 6E7EC14007D for ; Fri, 7 Nov 2014 09:19:26 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type; q=dns; s= default; b=gEp21iGqb9M+BQHp87DhobqKfab6kqG/MB7T80Xlnffes0Qv+9rWK ymnxGekPno6m4qlRG0zZRxEFUQ3GjiI0wc23Sp9Y7AOEyXJZm1B32Up8P0i9wtt8 A9hDngNHmumaSjcaqz2t6OEOEWvbEKxCm+7xKEnXVEVH0GQAC5qUW8= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type; s= default; bh=Z4XCduoYtpxwQBxMfl+jqFqL+zQ=; b=RLaqQqCXwraJHbxKovey 6yfqtPCb3OgDr3QIkAxKC1vLqUW2QjW1BrXqGtmfzrydt48R/Imeyh33c7jlgIuG WTe9y/NXnHWb+z8hdRbOBKTcm9pgt3Bm3AX1uAZbaY1sYdxwafMJPwduHRLMHAMF CejU3cSg4S3k+5SeQTJNwCQ= Received: (qmail 6428 invoked by alias); 6 Nov 2014 22:19:17 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 6416 invoked by uid 89); 6 Nov 2014 22:19:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 06 Nov 2014 22:19:14 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sA6MJC6R005099 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 6 Nov 2014 17:19:12 -0500 Received: from redhat.com (ovpn-116-30.ams2.redhat.com [10.36.116.30]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id sA6MJ9ff020668 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Thu, 6 Nov 2014 17:19:11 -0500 Date: Thu, 6 Nov 2014 23:19:08 +0100 From: Marek Polacek To: Jakub Jelinek , GCC Patches Subject: [PATCH] Don't necessarily emit object size checks for ARRAY_REFs Message-ID: <20141106221908.GE2342@redhat.com> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.23 (2014-03-12) First part of this patch is about removing the useless check that we talked about earlier today. The rest is about not emitting UBSAN_OBJECT_SIZE checks (those often come with multiple statements to compute a pointer difference) for ARRAY_REFs that are already instrumented by UBSAN_BOUNDS. I do this by moving the UBSAN_OBJECT_SIZE instrumentation so that it is done first in the ubsan pass - then I can just check whether the statement before that ARRAY_REF is a UBSAN_BOUND check. If it is, it should be clear that it is checking the ARRAY_REF, and I can drop the UBSAN_OBJECT_SIZE check. (Moving the UBSAN_OBJECT_SIZE instrumentation means that there won't be e.g. UBSAN_NULL check in between the ARRAY_REF and UBSAN_BOUND.) Earlier, I thought I should check that both UBSAN_OBJECT_SIZE and UBSAN_BOUND checks are checking the same array index, but that wouldn't work for multidimensional arrays, and just should not be needed. The new test shows where we were bogusly emitting both diagnostics and ensures that we only emit the -fsanitize=bounds one. Of course, if the test is run with -fsanitize=object-size or with -fsanitize=undefined -fno-sanitize=bounds, we emit the object size diagnostics and not the bounds diagnostics. Bootstrap-ubsan/regtest passed on x86_64-linux, ok for trunk? 2014-11-06 Marek Polacek * sanopt.c (sanopt_optimize_walker): Don't check alignment when popping a statement. * ubsan.c (instrument_object_size): Don't instrument already instrumented ARRAY_REFs. (pass_ubsan::execute): Perform object size instrumentation first. testsuite/ * c-c++-common/ubsan/object-size-9.c: Use -fsanitize=object-size. Add dg-output. * c-c++-common/ubsan/object-size-10.c: Likewise. * c-c++-common/ubsan/undefined-3.c: New test. Marek diff --git gcc/sanopt.c gcc/sanopt.c index 0fc032a..815d976 100644 --- gcc/sanopt.c +++ gcc/sanopt.c @@ -151,8 +151,7 @@ sanopt_optimize_walker (basic_block bb, struct sanopt_ctx *ctx) || flag_sanitize_undefined_trap_on_error || gimple_location (g) == gimple_location (stmt); - if (!remove && gimple_bb (g) == gimple_bb (stmt) - && tree_int_cst_compare (cur_align, align) == 0) + if (!remove && gimple_bb (g) == gimple_bb (stmt)) v.pop (); break; } diff --git gcc/testsuite/c-c++-common/ubsan/object-size-10.c gcc/testsuite/c-c++-common/ubsan/object-size-10.c index ebc8582..3e8608a 100644 --- gcc/testsuite/c-c++-common/ubsan/object-size-10.c +++ gcc/testsuite/c-c++-common/ubsan/object-size-10.c @@ -1,6 +1,6 @@ /* { dg-do run } */ /* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */ -/* { dg-options "-fsanitize=undefined" } */ +/* { dg-options "-fsanitize=object-size" } */ static char a[128]; static int b[128]; @@ -19,8 +19,7 @@ fn2 (int i) return a[i & 128]; } -/* { dg-output "index 128 out of bounds for type 'char \\\[128\\\]'\[^\n\r]*(\n|\r\n|\r)" } */ -/* { dg-output "\[^\n\r]*load of address \[^\n\r]* with insufficient space for an object of type 'char'\[^\n\r]*(\n|\r\n|\r)" } */ +/* { dg-output "load of address \[^\n\r]* with insufficient space for an object of type 'char'\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*note: pointer points here\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*\\^\[^\n\r]*(\n|\r\n|\r)" } */ @@ -39,7 +38,6 @@ fn4 (int i) return b[i & 128]; } -/* { dg-output "\[^\n\r]*index 128 out of bounds for type 'int \\\[128\\\]'\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*load of address \[^\n\r]* with insufficient space for an object of type 'int'\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*note: pointer points here\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*\[^\n\r]*(\n|\r\n|\r)" } */ @@ -52,7 +50,6 @@ fn5 (int i, int j) return b[i & j]; } -/* { dg-output "\[^\n\r]*index 128 out of bounds for type 'int \\\[128\\\]'\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*load of address \[^\n\r]* with insufficient space for an object of type 'int'\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*note: pointer points here\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*\[^\n\r]*(\n|\r\n|\r)" } */ diff --git gcc/testsuite/c-c++-common/ubsan/object-size-9.c gcc/testsuite/c-c++-common/ubsan/object-size-9.c index 829c822..842ad15 100644 --- gcc/testsuite/c-c++-common/ubsan/object-size-9.c +++ gcc/testsuite/c-c++-common/ubsan/object-size-9.c @@ -1,6 +1,6 @@ /* { dg-do run } */ /* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */ -/* { dg-options "-fsanitize=undefined" } */ +/* { dg-options "-fsanitize=object-size" } */ /* Test PARM_DECLs and RESULT_DECLs. */ @@ -35,7 +35,6 @@ f2 (int i) return x; } -/* { dg-output "\[^\n\r]*index 12 out of bounds for type 'char \\\[8\\\]'\[^\n\r]*(\n|\r\n|\r)" { target { c++ } } } */ /* { dg-output "\[^\n\r]*store to address \[^\n\r]* with insufficient space for an object of type 'char'\[^\n\r]*(\n|\r\n|\r)" { target { c++ } } } */ /* { dg-output "\[^\n\r]*note: pointer points here\[^\n\r]*(\n|\r\n|\r)" { target { c++ } } } */ /* { dg-output "\[^\n\r]*\[^\n\r]*(\n|\r\n|\r)" { target { c++ } } } */ @@ -64,7 +63,6 @@ f4 (int i) return s.d[i].b; } -/* { dg-output "\[^\n\r]*index 12 out of bounds for type 'U \\\[10\\\]'\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*load of address \[^\n\r]* with insufficient space for an object of type 'unsigned int'\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*note: pointer points here\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*\[^\n\r]*(\n|\r\n|\r)" } */ diff --git gcc/testsuite/c-c++-common/ubsan/undefined-3.c gcc/testsuite/c-c++-common/ubsan/undefined-3.c index e69de29..fc8900a 100644 --- gcc/testsuite/c-c++-common/ubsan/undefined-3.c +++ gcc/testsuite/c-c++-common/ubsan/undefined-3.c @@ -0,0 +1,67 @@ +/* { dg-do run } */ +/* { dg-options "-O2 -fsanitize=undefined -fno-sanitize-recover=object-size" } */ +/* Test that we only emit UBSAN_BOUNDS for the following ARRAY_REFs. */ + +static int g; +struct S { int a[10]; }; + +__attribute__((noinline, noclone)) void +fn1 (void) +{ + int a[10]; + asm ("" : : "r" (&a) : "memory"); + g = a[10]; +} + +/* { dg-output "index 10 out of bounds for type 'int \\\[10\\\]'\[^\n\r]*(\n|\r\n|\r)" } */ + +__attribute__((noinline, noclone)) void +fn2 (int i) +{ + int a[10]; + asm ("" : : "r" (&a) : "memory"); + g = a[i]; +} + +/* { dg-output "\[^\n\r]*index 10 out of bounds for type 'int \\\[10\\\]'\[^\n\r]*(\n|\r\n|\r)" } */ + +__attribute__((noinline, noclone)) void +fn3 (int i) +{ + int a[10][10][10]; + asm ("" : : "r" (&a) : "memory"); + g += a[i][5][5]; +} + +/* { dg-output "\[^\n\r]*index 10 out of bounds for type 'int \\\[10\\\]\\\[10\\\]\\\[10\\\]'\[^\n\r]*(\n|\r\n|\r)" } */ + +__attribute__((noinline, noclone)) void +fn4 (int i) +{ + struct S s; + asm ("" : : "r" (&s.a) : "memory"); + g = s.a[i]; +} + +/* { dg-output "\[^\n\r]*index 10 out of bounds for type 'int \\\[10\\\]'\[^\n\r]*(\n|\r\n|\r)" } */ + +__attribute__((noinline, noclone)) void +fn5 (int i) +{ + int a[10]; + a[1] = 10; + asm ("" : : "r" (&a) : "memory"); + g = a[a[1]]; +} + +/* { dg-output "\[^\n\r]*index 10 out of bounds for type 'int \\\[10\\\]'\[^\n\r]*(\n|\r\n|\r)" } */ + +int +main () +{ + fn1 (); + fn2 (10); + fn3 (10); + fn4 (10); + fn5 (10); +} diff --git gcc/ubsan.c gcc/ubsan.c index 41cf546..4a67801 100644 --- gcc/ubsan.c +++ gcc/ubsan.c @@ -1484,6 +1484,22 @@ instrument_object_size (gimple_stmt_iterator *gsi, bool is_lhs) || bitsize != size_in_bytes * BITS_PER_UNIT) return; + /* Don't instrument ARRAY_REFs if -fsanitize=bounds already + took care of that. */ + gimple_stmt_iterator gsi2 = *gsi; + if ((flag_sanitize & SANITIZE_BOUNDS) && !gsi_end_p (gsi2)) + { + /* Look at the previous statement. */ + gsi_prev (&gsi2); + gimple prev_stmt = gsi_stmt (gsi2); + if (prev_stmt && is_gimple_call (prev_stmt) + && gimple_call_internal_p (prev_stmt) + && gimple_call_internal_fn (prev_stmt) == IFN_UBSAN_BOUNDS) + /* Ok, we have at least one UBSAN_BOUNDS call that is checking + this very ARRAY_REF. No need to emit objsize checking. */ + return; + } + bool decl_p = DECL_P (inner); tree base = decl_p ? inner : TREE_OPERAND (inner, 0); tree ptr = build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (t)), t); @@ -1630,6 +1646,16 @@ pass_ubsan::execute (function *fun) continue; } + /* Perform this instrumentation first, so we can easily + check for UBSAN_BOUNDS before this statement. */ + if (flag_sanitize & SANITIZE_OBJECT_SIZE) + { + if (gimple_store_p (stmt)) + instrument_object_size (&gsi, true); + if (gimple_assign_load_p (stmt)) + instrument_object_size (&gsi, false); + } + if ((flag_sanitize & SANITIZE_SI_OVERFLOW) && is_gimple_assign (stmt)) instrument_si_overflow (gsi); @@ -1664,14 +1690,6 @@ pass_ubsan::execute (function *fun) bb = gimple_bb (stmt); } - if (flag_sanitize & SANITIZE_OBJECT_SIZE) - { - if (gimple_store_p (stmt)) - instrument_object_size (&gsi, true); - if (gimple_assign_load_p (stmt)) - instrument_object_size (&gsi, false); - } - gsi_next (&gsi); } }