From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 48674 invoked by alias); 23 Aug 2016 15:18:02 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 48589 invoked by uid 89); 23 Aug 2016 15:18:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=Dave, APIs, apis, UD:diagnostic.h X-HELO: mail-yw0-f169.google.com Received: from mail-yw0-f169.google.com (HELO mail-yw0-f169.google.com) (209.85.161.169) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 23 Aug 2016 15:17:51 +0000 Received: by mail-yw0-f169.google.com with SMTP id j12so73859608ywb.2 for ; Tue, 23 Aug 2016 08:17:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=VNmM03uYrgh4FCm8Ibmew4bGuOznZpex4te7l7VmWJY=; b=IrEpAe1WPxva4y3E7oeqwtg6LvcrQCnFtGd2dZbCkmrU/c8ZlUVPqPFevbIwKVaYPX 45XvyW9xI0OpKONI7kixuU8nDdr3+ywggodVyxYcLfqrCLBfCQQY/rlA+0CWlLdsay8s qWFW7+sPYXZiBjCzE4KXLPi/CUKQtMecMbcdGquMsjuRQRwrUctTlrHvsMJVLEdSnvQ6 vIO++Nu/my7Tv2GHwSfV9k33y7icjuZMPkOko71fGpzDxfOFTCt3+CXCEKfBuCp/eoZx wRQQpVsSex8P5/KRXdnNxNAjvLW67MFeSe+bP5lqXfiqHcFpAQ0PLckKO/ZWDm+NrHTM dLrQ== X-Gm-Message-State: AEkoouuAbljKNYqHiaHD4g0+9YKmTYaJBz/AibQPAn373VF2oJYlmtfxsWsQIkuYboPACQ== X-Received: by 10.129.153.210 with SMTP id q201mr21736273ywg.309.1471965469740; Tue, 23 Aug 2016 08:17:49 -0700 (PDT) Received: from [192.168.0.26] (97-122-170-38.hlrn.qwest.net. [97.122.170.38]) by smtp.gmail.com with ESMTPSA id w79sm2138286ywd.51.2016.08.23.08.17.48 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 23 Aug 2016 08:17:49 -0700 (PDT) Subject: Re: [PATCH] RFC: On-demand locations within string-literals To: David Malcolm , gcc-patches@gcc.gnu.org References: <1468014566-40305-1-git-send-email-dmalcolm@redhat.com> <5793E33F.2090305@gmail.com> <1469320654.29375.36.camel@redhat.com> <57BBC213.2040006@gmail.com> <1471960762.8216.34.camel@redhat.com> From: Martin Sebor Message-ID: <57BC691B.3010809@gmail.com> Date: Tue, 23 Aug 2016 15:18:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <1471960762.8216.34.camel@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2016-08/txt/msg01641.txt.bz2 On 08/23/2016 07:59 AM, David Malcolm wrote: > On Mon, 2016-08-22 at 21:25 -0600, Martin Sebor wrote: >>>> Beyond that, the range normally works fine, except when macros >>>> are involved like they are in my tests. You can see the effect >>>> in the range.out file. (This works without your patch but it >>>> could very well be because I didn't set it up right.) >>> >>> Sadly I can't figure out what's going wrong - but the code's >>> changed a >>> lot at my end since then. Sorry. >> >> I have integrated the latest (already committed) version of your >> patch into my -Wformat-length patch. Everything (well almost) >> works and I get nice ranges for the format string and for (some) >> arguments. >> >> I was surprised at how long it took me to switch from the previous >> implementation (also copied from c-format.c) to this new API. As >> before, I had to copy bits and pieces of code from other parts of >> the compiler to get it to work. I was also surprised at how complex >> making use of it is. It added 130 lines of code to the pass, which >> is 40 more than what I had before. It seems that the >> format_warning_at_substring function from c-format.c (perhaps >> generalized and with some reasonable defaults hardcoded) should >> be defined where other parts of GCC (including the middle end) >> can reuse it. > > I'm guessing that it was difficult because the most useful parts are > currently in c-format.c, whereas your code is in the middle-end. > > Is the latest version of your patch posted somewhere where I can see > it? I'm planning/hoping to post it this week. It was only difficult in that I had to gather bits from different parts of the compiler and figure out how to make them work together. I.e., it wasn't a simple matter of replacing one function call with another. In the end, it boiled down to replacing the get_location function with this: const char * substring_loc::get_location (location_t *out_loc) const { gcc_assert (out_loc); if (!g_string_concat_db) g_string_concat_db = new (ggc_alloc ()) string_concat_db (); static struct cpp_reader* parse_in; if (!parse_in) { /* Create and initialize a preprocessing reader. */ parse_in = cpp_create_reader (CLK_GNUC99, ident_hash, line_table); cpp_init_iconv (parse_in); } return get_source_location_for_substring (parse_in, g_string_concat_db, m_fmt_string_loc, CPP_STRING, m_caret_idx, m_start_idx, m_end_idx, out_loc); } > > The substring_loc class should probably be moved from c-family to gcc > also. We might need a langhook to support that though (not sure yet). > > I'd be up for doing these moves (maybe moving > format_warning_at_substring to diagnostic.h/c), but I'd prefer to see > your patch first. > Sure. > So would you like the output to look like this: > > > Option (a): underline whole string, with caret at close-quote > > t.c: In function ‘f’: > t.c:5:25: warning: writing a terminating nul past the end of the > destination [-Wformat-length=] > __builtin_sprintf (d, "%sX", "1"); > ~~~~^ > > or like this: > > Option (b): just the close-quote > > t.c: In function ‘f’: > t.c:5:25: warning: writing a terminating nul past > the end of the > destination [-Wformat-length=] > __builtin_sprintf (d, > "%sX", "1"); > ^ > ? > Either one would work for me. If this were to become a general purpose interface then I think it would be nice to let the caller decide which end/quote (if any) to point the caret at. > (do you also emit a note/inform showing the size of d?) Yes. The full diagnostic looks like this (with the care at the wrong end as we're discussing): t.c:5:25: warning: writing a terminating nul past the end of the destination [-Wformat-length=] __builtin_sprintf (d, "%sX", "A"); ^~~~~ t.c:5:3: note: format output 3 bytes into a destination of size 2 __builtin_sprintf (d, "%sX", "A"); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > What API are you using to emit the warning? I copied the format_warning_va and format_warning_at subscript functions from c-format.c and I'm calling the latter. I'm not using the hint for anything yet (not sure if there's an opportunity to make use of it). > Given the location of the > string as a whole expressed as a location_t, you can probably do > something like this: > > location_t > option_a (location_t string_as_a_whole_loc) > { > source_range src_range > = get_range_from_loc (line_table, string_as_a_whole_loc); > > return make_location (src_range.m_finish, /* caret */ > src_range.m_start, src_range.m_finish); > } > > location_t > option_b (location_t string_as_a_whole_loc) > { > source_range > src_range > = get_range_from_loc (line_table, string_as_a_whole_loc); > > return src_range.m_finish; > } > > (these could be added to libcpp) > > but I get the impression you want something like this integrated into > the format_warning or substring_loc APIs (and it's hard to tell without > seeing your patch). I guess I was hoping for a simple high level interface to warning_at where I could control the offset of the caret and the underlining within some bounds. Completely off the cuff, say if warning_at were overloaded to take another argument with the offsets, then something like this (with offsets in characters): void my_warning (location_t loc) { int caret = /* offset of caret from the beginning of loc */; int begin = /* optional offset of the start of underlining */; int end = /* optional offset of the end of underlining */; warning_at (range (loc, caret, begin, end), ...); } Maybe I should prototype it to understand if it can be done and what the tradeoffs might be. The most recently posted patch uses the location_from_offset function that c-format.c used before your changes: https://gcc.gnu.org/ml/gcc-patches/2016-08/msg00986.html In my latest patch I replaced the function and calls to warning_at with its result with format_warning_at_substring. I mainly did it because I thought that was going to be new/recommended API for this sort of thing. I wasn't having any problems with previous approach or looking for enhancements (though the split location for the format directive and its argument is nice). Did I misunderstand what the intent of your changes was? (I.e., did you not expect me to make that switch?) > > Hope this is helpful > Dave Yes, thanks. Martin