From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x742.google.com (mail-qk1-x742.google.com [IPv6:2607:f8b0:4864:20::742]) by sourceware.org (Postfix) with ESMTPS id B35C5386F465 for ; Mon, 22 Jun 2020 22:22:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B35C5386F465 Received: by mail-qk1-x742.google.com with SMTP id k18so7967251qke.4 for ; Mon, 22 Jun 2020 15:22:16 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=nkhY/LBbd36eIMD6L4UEphGF3YCLww0BMFSyB51RRrc=; b=LR78K3LQNhYhyvvpvUkfGB5l/eqBnll33BbwY2j1Md80m8/+/WaVqqGqjsTALWH9mj VmpyM0KIOfEZ56udec9nYoF3D+J0cAxea0mvy6PiZc3lddOrD2jn0x8rEE7mgCjCQdIb PyTyS2+nyaQ1UxvINtS4Q9uLZLxH41Wa4NOZf5iiDaJgxLMaNO0vq1s+qiUxoTawFHwY z5ue81KTHa3ogS2RjCr69qfe+o6jExDFRdk9xQxDgTqo1h0b/lfhDfOG8z2VXWCKOgJK eIG1W868/PsBvCEqpMjDscZ1JAErCiZ9bmqN2xzbSVkEvTV7IczSQJ5G3qDD1v/4hhh2 mOSw== X-Gm-Message-State: AOAM531KI6P7Cur+tB3Cdi85TPr7n4x57QB483ElSJykQqrIkT5I/OoN k56JHbjTyEAN9/SBkBKJV+fET4Fg X-Google-Smtp-Source: ABdhPJw3aENCqeMbQ1PjxmDphX/RBcLqWRqlBIk2tEzEcGlc4mKNupoDRCLZ1ToG9NyalAw97G0mEA== X-Received: by 2002:a37:bd84:: with SMTP id n126mr7399952qkf.310.1592864535902; Mon, 22 Jun 2020 15:22:15 -0700 (PDT) Received: from [192.168.0.41] (184-96-233-25.hlrn.qwest.net. [184.96.233.25]) by smtp.gmail.com with ESMTPSA id f30sm15704876qtg.79.2020.06.22.15.22.14 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 22 Jun 2020 15:22:15 -0700 (PDT) Subject: Re: [PATCH] handle MEM_REF with void* arguments (PR c++/95768) To: Jason Merrill , gcc-patches References: From: Martin Sebor Message-ID: <29c9b3fa-69a2-dca9-1477-54aac80c8680@gmail.com> Date: Mon, 22 Jun 2020 16:22:13 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------04A3398B34633EE63B7F70F2" Content-Language: en-US X-Spam-Status: No, score=-8.4 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 22 Jun 2020 22:22:18 -0000 This is a multi-part message in MIME format. --------------04A3398B34633EE63B7F70F2 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit On 6/22/20 12:55 PM, Jason Merrill wrote: > On 6/22/20 1:25 PM, Martin Sebor wrote: >> The attached fix parallels the one for the equivalent C bug 95580 >> where the pretty printers don't correctly handle MEM_REF arguments >> with type void* or other pointers to an incomplete type. >> >> The incorrect handling was exposed by the recent change to >> -Wuninitialized which includes such expressions in diagnostics. > >> +        if (tree size = TYPE_SIZE_UNIT (TREE_TYPE (argtype))) >> +          if (!integer_onep (size)) >> +            { >> +              pp_cxx_left_paren (pp); >> +              dump_type (pp, ptr_type_node, flags); >> +              pp_cxx_right_paren (pp); >> +            } > > Don't we want to print the cast if the pointer target type is incomplete? I suppose, yes, although after some more testing I think what should be output is the type of the access. The target pointer type isn't meaningful (at least not in this case). Here's what the warning looks like in C for the test case in gcc.dg/pr95580.c: warning: ‘*((void *)(p)+1)’ may be used uninitialized and like this in C++: warning: ‘*(p +1)’ may be used uninitialized The +1 is a byte offset, which is correct given that incrementing a void* in GCC is the same as adding 1 to the byte address, but dereferencing a void* doesn't correspond to what's going on in the source. Even for a complete type (with size greater than 1), printing the type of the argument plus a byte offset is wrong. It ends up with this for the C++ test case from 95768: warning: ‘*((int*) +4)’ is used uninitialized when the access is actually ‘*((int*) +1)’ So it seems to me for MEM_REF, to make the output meaningful, it's the type of the access (i.e., the MEM_REF type) that should be printed here, and the offset should either be in elements of the accessed type, i.e., warning: ‘*((int*) +1)’ is used uninitialized or, if the access is misaligned, the argument should first be cast to char*, the offset added, and the result then cast to the access type, like this: warning: ‘*(T*)((char*) +1)’ is used uninitialized The attached revised and less than fully tested patch implements this for C++ only for now. If we agree on this approach I'll see about making the corresponding change in C. Martin --------------04A3398B34633EE63B7F70F2 Content-Type: text/x-patch; name="gcc-95768.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gcc-95768.diff" PR c++/95768 - pretty-printer ICE on -Wuninitialized with allocated storage gcc/cp/ChangeLog: PR c++/95768 * error.c (dump_expr): Handle sizeless operand types such as void*. gcc/testsuite/ChangeLog: PR c++/95768 * g++.dg/pr95768.C: New test. diff --git a/gcc/cp/error.c b/gcc/cp/error.c index 0d6375e5e14..ea1f3232e3d 100644 --- a/gcc/cp/error.c +++ b/gcc/cp/error.c @@ -2374,32 +2374,63 @@ dump_expr (cxx_pretty_printer *pp, tree t, int flags) break; case MEM_REF: - if (TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR - && integer_zerop (TREE_OPERAND (t, 1))) - dump_expr (pp, TREE_OPERAND (TREE_OPERAND (t, 0), 0), flags); - else - { - pp_cxx_star (pp); - if (!integer_zerop (TREE_OPERAND (t, 1))) - { - pp_cxx_left_paren (pp); - if (!integer_onep (TYPE_SIZE_UNIT - (TREE_TYPE (TREE_TYPE (TREE_OPERAND (t, 0)))))) - { - pp_cxx_left_paren (pp); - dump_type (pp, ptr_type_node, flags); - pp_cxx_right_paren (pp); - } - } - dump_expr (pp, TREE_OPERAND (t, 0), flags); - if (!integer_zerop (TREE_OPERAND (t, 1))) - { - pp_cxx_ws_string (pp, "+"); - dump_expr (pp, fold_convert (ssizetype, TREE_OPERAND (t, 1)), - flags); - pp_cxx_right_paren (pp); - } - } + { + tree type = TREE_TYPE (t); + tree arg = TREE_OPERAND (t, 0); + tree offset = TREE_OPERAND (t, 1); + bool zero_offset = integer_zerop (offset); + + if (TREE_CODE (arg) == ADDR_EXPR && zero_offset) + dump_expr (pp, TREE_OPERAND (arg, 0), flags); + else + { + pp_cxx_star (pp); + if (!zero_offset) + { + pp_cxx_left_paren (pp); + pp_cxx_left_paren (pp); + dump_type (pp, type, flags); + pp_cxx_star (pp); + pp_cxx_right_paren (pp); + + bool byte_offset = true; + + if (tree size = TYPE_SIZE_UNIT (type)) + { + /* For naturally aligned accesses print the nonzero + offset in units of the access type. For unaligned + accesses print a byte offset instead. */ + offset_int wsiz = wi::to_offset (size); + offset_int woff = wi::to_offset (offset); + offset_int szlg2 = wi::floor_log2 (wsiz); + offset_int eltoff = woff >> szlg2; + if (eltoff << szlg2 == woff) + { + offset = wide_int_to_tree (ssizetype, eltoff); + byte_offset = false; + } + } + + if (byte_offset) + { + /* When printing the byte offset include a cast to + a character type first, before printing the cast + to the access type. */ + pp_cxx_left_paren (pp); + dump_type (pp, char_type_node, flags); + pp_cxx_star (pp); + pp_cxx_right_paren (pp); + } + } + dump_expr (pp, arg, flags); + if (!zero_offset) + { + pp_cxx_ws_string (pp, "+"); + dump_expr (pp, fold_convert (ssizetype, offset), flags); + pp_cxx_right_paren (pp); + } + } + } break; case NEGATE_EXPR: diff --git a/gcc/testsuite/g++.dg/pr95768.C b/gcc/testsuite/g++.dg/pr95768.C new file mode 100644 index 00000000000..23e6988b410 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr95768.C @@ -0,0 +1,32 @@ +/* PR c++/95768 - pretty-printer ICE on -Wuninitialized with allocated storage + { dg-do compile } + { dg-options "-O2 -Wall" } */ + +extern "C" void *malloc (__SIZE_TYPE__); + +struct f +{ + int i; + static int e (int); + void operator= (int) { e (i); } +}; + +struct m { + int i; + f length; +}; + +struct n { + m *o() { return (m *)this; } +}; + +struct p { + n *header; + p () { + header = (n *)malloc (0); + m b = *header->o(); // { dg-warning "'\\*\\(\\(int\\*\\)<\[a-z\]+> \\+1\\)' is used uninitialized" } + b.length = 0; + } +}; + +void detach2() { p(); } diff --git a/gcc/testsuite/g++.dg/warn/Wuninitialized-11.C b/gcc/testsuite/g++.dg/warn/Wuninitialized-11.C new file mode 100644 index 00000000000..6f8a4586adf --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wuninitialized-11.C @@ -0,0 +1,40 @@ +/* Verify that -Wuninitialized warnings about accesses to objects via + pointers and offsets mention valid expressions. + { dg-do compile } + { dg-options "-O2 -Wall" } */ + +typedef __INT16_TYPE__ int16_t; +typedef __INT32_TYPE__ int32_t; + +void sink (int); + +/* Verify properly aligned accesses at offsets that are multiples of + the access size. */ + +void test_aligned (void) +{ + char *p1 = (char*)__builtin_malloc (32); + p1 += sizeof (int32_t); + + int16_t *p2 = (int16_t*)p1; + sink (p2[1]); // { dg-warning "'\\\*\\\(\\\(int16_t\\\*\\\)p1 \\\+ *3\\\)' is used uninitialized" } + + int32_t *p4 = (int32_t*)p1; + sink (p4[1]); // { dg-warning "'\\\*\\\(\\\(int32_t\\\*\\\)p1 \\\+ *2\\\)' is used uninitialized" } +} + + +/* Verify misaligned accesses at offsets that aren't multiples of + the access size. */ + +void test_misaligned (void) +{ + char *p1 = (char*)__builtin_malloc (32); + p1 += 1; + + int16_t *p2 = (int16_t*)p1; + sink (p2[1]); // { dg-warning "'\\\*\\\(\\\(int16_t\\\*\\\)\\\(char\\\*\\\)p1 \\\+ *3\\\)' is used uninitialized" } + + int32_t *p4 = (int32_t*)p1; + sink (p4[1]); // { dg-warning "'\\\*\\\(\\\(int32_t\\\*\\)\\\(char\\\*\\\)p1 \\\+ *5\\\)' is used uninitialized" } +} --------------04A3398B34633EE63B7F70F2--