From patchwork Thu Aug 15 11:04:03 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marek Polacek X-Patchwork-Id: 267351 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 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "localhost", Issuer "www.qmailtoaster.com" (not verified)) by ozlabs.org (Postfix) with ESMTPS id CC15D2C0128 for ; Thu, 15 Aug 2013 21:04:18 +1000 (EST) 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:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=NoooWs4X7NqckoKiL emQEgYX6zgv1phW5t8Jw1MWgW13xDLKypBmXDILJpbf/bnrzFBoaQSGAR+zVlpVl /ueKF4YwVPm7S0CJb+qokla9nebxqYKVHk/7mIf5ENrB70918gnDOBh3ShfW6A6I lo0Ml22ScqpQ8wgB0VoTqXW4WI= 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:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=/hzCgXrhQnM8+xg4CpSDrks xhsg=; b=WE2vTeZJodfUpIjGKoLwcG6056Rln2sSx/2U9uAIjoz+rMbY3fgwglu 7O3q1WJate1qNXavKKYx+yvmubXTXcdlM/2N++0LRYJyL5bYls2VFmW+VSEkEWZ/ 8uphfLQYf2oejIH8wzRHwKXUgCad6E5qFE3XFsGFi2NBdhYprdtE= Received: (qmail 30609 invoked by alias); 15 Aug 2013 11:04:12 -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 30600 invoked by uid 89); 15 Aug 2013 11:04:11 -0000 X-Spam-SWARE-Status: No, score=-7.0 required=5.0 tests=AWL, BAYES_00, RCVD_IN_HOSTKARMA_W, RCVD_IN_HOSTKARMA_WL, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Thu, 15 Aug 2013 11:04:10 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7FB47FH028677 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 15 Aug 2013 07:04:08 -0400 Received: from redhat.com (ovpn-116-26.ams2.redhat.com [10.36.116.26]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r7FB43Q6024233 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 15 Aug 2013 07:04:05 -0400 Date: Thu, 15 Aug 2013 13:04:03 +0200 From: Marek Polacek To: Jason Merrill Cc: GCC Patches , Jakub Jelinek , Jeff Law , "Joseph S. Myers" Subject: Re: Request to merge Undefined Behavior Sanitizer in (take 2) Message-ID: <20130815110403.GY17022@redhat.com> References: <20130725153227.GC32538@redhat.com> <51F186E9.9090404@redhat.com> <20130731173321.GT17022@redhat.com> <51F95CF7.8020605@redhat.com> <20130801180615.GU17022@redhat.com> <51FD2EC0.4060607@redhat.com> <20130805112452.GX17022@redhat.com> <52016978.8010807@redhat.com> <20130807145803.GE17022@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20130807145803.GE17022@redhat.com> User-Agent: Mutt/1.5.20 (2009-06-14) On Wed, Aug 07, 2013 at 04:58:03PM +0200, Marek Polacek wrote: > I actually don't know what I prefer more, but if you like this version > more, I'll go with it. Maybe this is better because we don't have to > create SAVE_EXPR and also we avoid one fold_build call. Thanks, Not creating the SAVE_EXPRs turned to be a no-go: firstly, the logic when to create the SAVE_EXPRs and when not is hard-to-follow (depends on C/C++, depends on various standards and it also depends on whether the types are signed or not) - both for shift and division. Moreover, when we have nested expressions like e.g. (i << u) << x if any of the operands is wrapped in SAVE_EXPR, we get an -Wuninitialized warning. So, what I did is to evaluate the op0 always in the condition, like "if (op0, )", which is safe and all the uninitialized warnings are gone, now the bootstrap with -fsanitize=undefined finally finishes. (albeit with Comparing stages 2 and 3 warning: gcc/cc1plus-checksum.o differs warning: gcc/cc1-checksum.o differs Bootstrap comparison failure! gcc/combine.o differs gcc/tree-ssa-loop-ivopts.o differs gcc/rtlanal.o differs gcc/calls.o differs gcc/emit-rtl.o differs gcc/dbxout.o differs gcc/function.o differs gcc/cfgexpand.o differs gcc/stor-layout.o differs gcc/tree.o differs gcc/tree-vect-generic.o differs gcc/expr.o differs gcc/fixed-value.o differs gcc/fold-const.o differs gcc/i386.o differs libdecnumber/decimal64.o differs libdecnumber/decNumber.o differs make[2]: *** [compare] Error 1 ). Tested x86_64-pc-linux-gnu, applying to the ubsan branch. 2013-08-15 Marek Polacek * c-ubsan.c (ubsan_instrument_division): Evaluate SAVE_EXPRs before the condition. (ubsan_instrument_shift): Likewise. * c-c++-common/ubsan/save-expr-1.c: New test. * c-c++-common/ubsan/save-expr-2.c: New test. * c-c++-common/ubsan/save-expr-3.c: New test. * c-c++-common/ubsan/save-expr-4.c: New test. Marek --- gcc/c-family/c-ubsan.c.mp 2013-08-15 10:44:51.189966522 +0200 +++ gcc/c-family/c-ubsan.c 2013-08-15 12:43:30.454523049 +0200 @@ -73,6 +73,10 @@ ubsan_instrument_division (location_t lo x = fold_build2 (TRUTH_AND_EXPR, boolean_type_node, x, tt); t = fold_build2 (TRUTH_OR_EXPR, boolean_type_node, t, x); } + + /* In case we have a SAVE_EXPR in a conditional context, we need to + make sure it gets evaluated before the condition. */ + t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), op0, t); tree data = ubsan_create_data ("__ubsan_overflow_data", loc, ubsan_type_descriptor (type), NULL_TREE); @@ -133,6 +137,10 @@ ubsan_instrument_shift (location_t loc, build_int_cst (type0, 0)); tt = fold_build2 (TRUTH_OR_EXPR, boolean_type_node, x, tt); } + + /* In case we have a SAVE_EXPR in a conditional context, we need to + make sure it gets evaluated before the condition. */ + t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), op0, t); tree data = ubsan_create_data ("__ubsan_shift_data", loc, ubsan_type_descriptor (type0), ubsan_type_descriptor (type1), NULL_TREE); --- gcc/testsuite/c-c++-common/ubsan/save-expr-1.c.mp 2013-08-15 10:45:37.377057713 +0200 +++ gcc/testsuite/c-c++-common/ubsan/save-expr-1.c 2013-08-15 10:53:01.601695980 +0200 @@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-options "-fsanitize=shift -Wall -Werror -O" } */ + +static int x; +int +main (void) +{ + int o = 1; + int y = x << o; + return y; +} --- gcc/testsuite/c-c++-common/ubsan/save-expr-2.c.mp 2013-08-15 10:58:37.505938910 +0200 +++ gcc/testsuite/c-c++-common/ubsan/save-expr-2.c 2013-08-15 10:58:30.258913139 +0200 @@ -0,0 +1,14 @@ +/* { dg-do compile } */ +/* { dg-options "-fsanitize=shift -Wall -Werror -O" } */ + +int +foo (int i, unsigned int u) +{ + return u / i; +} + +int +bar (int i, unsigned int u) +{ + return u % i; +} --- gcc/testsuite/c-c++-common/ubsan/save-expr-3.c.mp 2013-08-15 11:02:39.111142875 +0200 +++ gcc/testsuite/c-c++-common/ubsan/save-expr-3.c 2013-08-15 11:02:34.874127652 +0200 @@ -0,0 +1,16 @@ +/* { dg-do compile } */ +/* { dg-options "-fsanitize=shift -Wall -Werror -O" } */ + +int x; + +int +foo (int i, int u) +{ + return (i << u) << x; +} + +int +bar (int i, int u) +{ + return (i >> u) >> x; +} --- gcc/testsuite/c-c++-common/ubsan/save-expr-4.c.mp 2013-08-15 11:09:53.453746629 +0200 +++ gcc/testsuite/c-c++-common/ubsan/save-expr-4.c 2013-08-15 11:09:45.238716728 +0200 @@ -0,0 +1,16 @@ +/* { dg-do compile } */ +/* { dg-options "-fsanitize=shift -Wall -Werror -O" } */ + +int x; + +int +foo (int i, unsigned int u) +{ + return (i % u) << (x / u); +} + +int +bar (int i, unsigned int u) +{ + return (((x % u) << (u / i)) >> x); +}