From patchwork Wed Dec 16 18:04:04 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 557626 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 1DE391401AD for ; Thu, 17 Dec 2015 04:44:43 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=vIx4hcJs; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id:mime-version:content-type :content-transfer-encoding; q=dns; s=default; b=s7BnE7coOWLRhPrd lvACYRL5CvfEdCR4034klmUria+x6xQO2o/BIz3OxwCbfsJ7KHQPNzgXY2+7LYQ9 UYu3Blu/9D+/LTaD4SSE8M6JHxVYPEA6FF5VWhuD6HqUWelzDWzGjivnSQ6c6YxF TUjkxhB1XocCYGSjfEBkfXZum4M= 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:from :to:cc:subject:date:message-id:mime-version:content-type :content-transfer-encoding; s=default; bh=9mZi1fRbrkHYfvFXjsSaIv ROtZs=; b=vIx4hcJs1BcSyW4BZ8MtZZIVLPBzvXzi+j/VSEia9FboX/8FaLXPuo nXXkhhrPXxz3wTP8IZno/Yx98w9sN0shqCBXmwh2jxevnMBQ76u9gDRJBc/Jzojf 06yr1kGnXGBFzz8sRIV2HA3OFYS0eq/4y+mIiVNWObESJAoKA9u5o= Received: (qmail 18074 invoked by alias); 16 Dec 2015 17:44:37 -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 18063 invoked by uid 89); 16 Dec 2015 17:44:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL, BAYES_00, KAM_ASCII_DIVIDERS, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=LOCATION, 1, 44, 486, sk:ef64e6b 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; Wed, 16 Dec 2015 17:44:26 +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 (Postfix) with ESMTPS id 4CDC6C0B7E0A for ; Wed, 16 Dec 2015 17:44:25 +0000 (UTC) Received: from c64.redhat.com (vpn-238-44.phx2.redhat.com [10.3.238.44]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tBGHiOum031376; Wed, 16 Dec 2015 12:44:24 -0500 From: David Malcolm To: gcc-patches@gcc.gnu.org Cc: David Malcolm Subject: [PATCH] C and C++ FE: better source ranges for binary ops Date: Wed, 16 Dec 2015 13:04:04 -0500 Message-Id: <1450289044-35720-1-git-send-email-dmalcolm@redhat.com> MIME-Version: 1.0 X-IsSubscribed: yes Currently trunk emits range information for most bad binary operations in the C++ frontend; but not in the C frontend. The helper function binary_op_error shared by C and C++ takes a location_t. In the C++ frontend, a location_t containing the range has already been built, so we get the underline when it later validates the expression: e.g. this example from: https://gcc.gnu.org/wiki/ClangDiagnosticsComparison t.cc:9:19: error: no match for ‘operator+’ (operand types are ‘int’ and ‘foo’) return P->bar() + *P; ~~~~~~~~~^~~~ In the C frontend, the "full" location_t is only built after type-checking, so if type-checking fails, we get a caret/range covering just the operator so e.g. for: return (some_function () + some_other_function ()); we get just: gcc.dg/bad-binary-ops.c:29:4: error: invalid operands to binary + (have 'struct s' and 'struct t') + some_other_function ()); ^ The following patch updates binary_op_error to accept a rich_location *. For the C++ frontend we populate it with just the location_t as before, but for the C frontend we can add locations for the operands, giving this underlining for the example above: bad-binary-ops.c:29:4: error: invalid operands to binary + (have 'struct s' and 'struct t') return (some_function () ~~~~~~~~~~~~~~~~ + some_other_function ()); ^ ~~~~~~~~~~~~~~~~~~~~~~ Additionally, in the C++ frontend, cp_build_binary_op has an "error" call, which implicitly uses input_location, giving this reduced information for another test case from https://gcc.gnu.org/wiki/ClangDiagnosticsComparison: bad-binary-ops.C:10:14: error: invalid operands of types '__m128 {aka float}' and 'const int*' to binary 'operator/' myvec[1] / ptr; ^~~ The patch updates it to use error_at with the location_t provided, fixing the above to become: bad-binary-ops.C:10:12: error: invalid operands of types '__m128 {aka float}' and 'const int*' to binary 'operator/' myvec[1] / ptr; ~~~~~~~~~^~~~~ Finally, since this patch adds a usage of gcc_rich_location::maybe_add_expr to cc1, we can drop the hacked-in copy of gcc_rich_location::add_expr from gcc.dg/plugin/diagnostic_plugin_show_trees.c. Successfully bootstrapped®rtested on x86_64-pc-linux-gnu; adds 15 new PASS results in g++.sum and 7 new PASS results in gcc.sum OK for trunk in stage 3? gcc/c-family/ChangeLog: * c-common.c (binary_op_error): Convert first param from location_t to rich_location * and use it when emitting an error. * c-common.h (binary_op_error): Convert first param from location_t to rich_location *. gcc/c/ChangeLog: * c-typeck.c: Include "gcc-rich-location.h". (build_binary_op): In the two places that call binary_op_error, create a gcc_rich_location and populate it with the location of the binary op and its two operands. gcc/cp/ChangeLog: * typeck.c (cp_build_binary_op): Update for change in signature of build_binary_op. Use error_at to replace an implicit use of input_location with param "location" in "invalid operands" error. (cp_build_binary_op): Replace an error with an error_at, using "location", rather than implicitly using input_location. gcc/testsuite/ChangeLog: * g++.dg/diagnostic/bad-binary-ops.C: New test case. * gcc.dg/bad-binary-ops.c: New test case. gcc.dg/plugin/diagnostic_plugin_show_trees.c (get_range_for_expr): Remove material copied from gcc-rich-location.c (gcc_rich_location::add_expr): Likewise. --- gcc/c-family/c-common.c | 21 +++++++--- gcc/c-family/c-common.h | 2 +- gcc/c/c-typeck.c | 11 ++++- gcc/cp/typeck.c | 13 ++++-- gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C | 44 ++++++++++++++++++++ gcc/testsuite/gcc.dg/bad-binary-ops.c | 48 ++++++++++++++++++++++ .../gcc.dg/plugin/diagnostic_plugin_show_trees.c | 44 -------------------- 7 files changed, 128 insertions(+), 55 deletions(-) create mode 100644 gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C create mode 100644 gcc/testsuite/gcc.dg/bad-binary-ops.c diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 4250cdf..653d1dc 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -3795,10 +3795,21 @@ c_register_builtin_type (tree type, const char* name) /* Print an error message for invalid operands to arith operation CODE with TYPE0 for operand 0, and TYPE1 for operand 1. - LOCATION is the location of the message. */ + RICHLOC is a rich location for the message, containing either + three separate locations for each of the operator and operands + + lhs op rhs + ~~~ ^~ ~~~ + + (C FE), or one location ranging over all over them + + lhs op rhs + ~~~~^~~~~~ + + (C++ FE). */ void -binary_op_error (location_t location, enum tree_code code, +binary_op_error (rich_location *richloc, enum tree_code code, tree type0, tree type1) { const char *opname; @@ -3850,9 +3861,9 @@ binary_op_error (location_t location, enum tree_code code, default: gcc_unreachable (); } - error_at (location, - "invalid operands to binary %s (have %qT and %qT)", opname, - type0, type1); + error_at_rich_loc (richloc, + "invalid operands to binary %s (have %qT and %qT)", + opname, type0, type1); } /* Given an expression as a tree, return its original type. Do this diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index ef64e6b..2d434b2 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -817,7 +817,7 @@ extern tree c_sizeof_or_alignof_type (location_t, tree, bool, bool, int); extern tree c_alignof_expr (location_t, tree); /* Print an error message for invalid operands to arith operation CODE. NOP_EXPR is used as a special case (see truthvalue_conversion). */ -extern void binary_op_error (location_t, enum tree_code, tree, tree); +extern void binary_op_error (rich_location *, enum tree_code, tree, tree); extern tree fix_string_type (tree); extern void constant_expression_warning (tree); extern void constant_expression_error (tree); diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c index 9d6c604..b93da07 100644 --- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -48,6 +48,7 @@ along with GCC; see the file COPYING3. If not see #include "cilk.h" #include "gomp-constants.h" #include "spellcheck.h" +#include "gcc-rich-location.h" /* Possible cases of implicit bad conversions. Used to select diagnostic messages in convert_for_assignment. */ @@ -11190,7 +11191,10 @@ build_binary_op (location_t location, enum tree_code code, && (!tree_int_cst_equal (TYPE_SIZE (type0), TYPE_SIZE (type1)) || !vector_types_compatible_elements_p (type0, type1))) { - binary_op_error (location, code, type0, type1); + gcc_rich_location richloc (location); + richloc.maybe_add_expr (orig_op0); + richloc.maybe_add_expr (orig_op1); + binary_op_error (&richloc, code, type0, type1); return error_mark_node; } @@ -11429,7 +11433,10 @@ build_binary_op (location_t location, enum tree_code code, if (!result_type) { - binary_op_error (location, code, TREE_TYPE (op0), TREE_TYPE (op1)); + gcc_rich_location richloc (location); + richloc.maybe_add_expr (orig_op0); + richloc.maybe_add_expr (orig_op1); + binary_op_error (&richloc, code, TREE_TYPE (op0), TREE_TYPE (op1)); return error_mark_node; } diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c index 39c1af2..ef9ae9a 100644 --- a/gcc/cp/typeck.c +++ b/gcc/cp/typeck.c @@ -4908,7 +4908,13 @@ cp_build_binary_op (location_t location, || !vector_types_compatible_elements_p (type0, type1)) { if (complain & tf_error) - binary_op_error (location, code, type0, type1); + { + /* "location" already embeds the locations of the + operands, so we don't need to add them separately + to richloc. */ + rich_location richloc (line_table, location); + binary_op_error (&richloc, code, type0, type1); + } return error_mark_node; } arithmetic_types_p = 1; @@ -4931,8 +4937,9 @@ cp_build_binary_op (location_t location, if (!result_type) { if (complain & tf_error) - error ("invalid operands of types %qT and %qT to binary %qO", - TREE_TYPE (orig_op0), TREE_TYPE (orig_op1), code); + error_at (location, + "invalid operands of types %qT and %qT to binary %qO", + TREE_TYPE (orig_op0), TREE_TYPE (orig_op1), code); return error_mark_node; } diff --git a/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C b/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C new file mode 100644 index 0000000..4ab7656 --- /dev/null +++ b/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C @@ -0,0 +1,44 @@ +// { dg-options "-fdiagnostics-show-caret" } + +// Adapted from https://gcc.gnu.org/wiki/ClangDiagnosticsComparison + +typedef float __m128; +void test_1 () +{ + __m128 myvec[2]; + int const *ptr; + myvec[1] / ptr; // { dg-error "invalid operands" } + +/* { dg-begin-multiline-output "" } + myvec[1] / ptr; + ~~~~~~~~~^~~~~ + { dg-end-multiline-output "" } */ +} + +struct s {}; +struct t {}; +extern struct s some_function (void); +extern struct t some_other_function (void); + +int test_2 (void) +{ + return (some_function () + + some_other_function ()); // { dg-error "no match for .operator" } + +/* { dg-begin-multiline-output "" } + return (some_function () + ~~~~~~~~~~~~~~~~ + + some_other_function ()); + ^~~~~~~~~~~~~~~~~~~~~~~~ + { dg-end-multiline-output "" } */ +} + +int test_3 (struct s param_s, struct t param_t) +{ + return param_s && param_t; // { dg-error "no match for .operator" } + +/* { dg-begin-multiline-output "" } + return param_s && param_t; + ~~~~~~~~^~~~~~~~~~ + { dg-end-multiline-output "" } */ +} diff --git a/gcc/testsuite/gcc.dg/bad-binary-ops.c b/gcc/testsuite/gcc.dg/bad-binary-ops.c new file mode 100644 index 0000000..e1da4d6 --- /dev/null +++ b/gcc/testsuite/gcc.dg/bad-binary-ops.c @@ -0,0 +1,48 @@ +/* { dg-options "-fdiagnostics-show-caret" } */ + +/* Adapted from https://gcc.gnu.org/wiki/ClangDiagnosticsComparison */ + +typedef float __m128; +void test_1 () +{ + __m128 myvec[2]; + int const *ptr; + myvec[1]/ptr; /* { dg-error "invalid operands to binary /" } */ + +/* TODO: ideally we'd underline "ptr" as well. +{ dg-begin-multiline-output "" } + myvec[1]/ptr; + ~~~~~~~~^ +{ dg-end-multiline-output "" } */ + + +} + +struct s {}; +struct t {}; +extern struct s some_function (void); +extern struct t some_other_function (void); + +int test_2 (void) +{ + return (some_function () + + some_other_function ()); /* { dg-error "invalid operands to binary \+" } */ + +/* { dg-begin-multiline-output "" } + return (some_function () + ~~~~~~~~~~~~~~~~ + + some_other_function ()); + ^ ~~~~~~~~~~~~~~~~~~~~~~ + { dg-end-multiline-output "" } */ +} + +int test_3 (struct s param_s, struct t param_t) +{ + return param_s + param_t; // { dg-error "invalid operands to binary \+" } + +/* { dg-begin-multiline-output "" } + return param_s + param_t; + ^ + { dg-end-multiline-output "" } */ +/* TODO: ideally we'd underline both params here. */ +} diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_show_trees.c b/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_show_trees.c index 5a911c1..c98034f 100644 --- a/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_show_trees.c +++ b/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_show_trees.c @@ -32,50 +32,6 @@ #include "gcc-rich-location.h" #include "print-tree.h" -/* - Hack: fails with linker error: -./diagnostic_plugin_show_trees.so: undefined symbol: _ZN17gcc_rich_location8add_exprEP9tree_node - since nothing in the tree is using gcc_rich_location::add_expr yet. - - I've tried various workarounds (adding DEBUG_FUNCTION to the - method, taking its address), but can't seem to fix it that way. - So as a nasty workaround, the following material is copied&pasted - from gcc-rich-location.c: */ - -static bool -get_range_for_expr (tree expr, location_range *r) -{ - if (EXPR_HAS_RANGE (expr)) - { - source_range sr = EXPR_LOCATION_RANGE (expr); - - /* Do we have meaningful data? */ - if (sr.m_start && sr.m_finish) - { - r->m_start = expand_location (sr.m_start); - r->m_finish = expand_location (sr.m_finish); - return true; - } - } - - return false; -} - -/* Add a range to the rich_location, covering expression EXPR. */ - -void -gcc_rich_location::add_expr (tree expr) -{ - gcc_assert (expr); - - location_range r; - r.m_show_caret_p = false; - if (get_range_for_expr (expr, &r)) - add_range (&r); -} - -/* FIXME: end of material taken from gcc-rich-location.c */ - int plugin_is_GPL_compatible; static void