From patchwork Wed Mar 23 12:41:53 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Biener X-Patchwork-Id: 601212 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 3qVTjM28BCz9snm for ; Wed, 23 Mar 2016 23:42:14 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=rCc1vTZ6; 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 :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=UPpjksoskiRkl9dgn1 e8dmWa3UyfWrvwc9TNtpUNRsGMLvXtmR6fxMswvVeLoibW/htUMK3DL/ZM9m+mVb fshlbcxnLjuudNARrin2949EHICQv18hTW+94WTdKHiZ21ynsnqd4gEDYnBcZl/b rLfrxfuOqWD9W+vdv31k3e6gs= 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 :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; s=default; bh=ZLf+nJOW5QNVAplynJyNfGOF QGk=; b=rCc1vTZ6TkvdUTS7K1TBWpNjSjyr+62UM9Pm+475ysdFl8DBhZxvsOxL n81IHZjwKpV5HJdH5nlKQbQUj9ygqJJZk0Rh2Gy0iC+Ueia4Rmus8WPx+uSGyzct tuGV6NeDgdIjGDUx2p3LdJgp+YwXLLpDEmClQ2U2HXFUDTM2JKE= Received: (qmail 9703 invoked by alias); 23 Mar 2016 12:42:04 -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 9614 invoked by uid 89); 23 Mar 2016 12:42:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=issuing, crude, worries, th X-HELO: mail-wm0-f41.google.com Received: from mail-wm0-f41.google.com (HELO mail-wm0-f41.google.com) (74.125.82.41) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 23 Mar 2016 12:41:56 +0000 Received: by mail-wm0-f41.google.com with SMTP id r129so134367644wmr.1 for ; Wed, 23 Mar 2016 05:41:56 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=5gE5HcflEGH6H6/72hGWd73jmmWmpnHnasyBlhEaAhI=; b=RX4RxUui/CkwvUl9DL/mxCr9jLzwJeJtd66YQtmVoW2lWgNsb3gAlUs9DglYDKC5ft +PgRXbYnNlGU3354lNzdhuzgiO8pqIN7abPNZgyWxol9JWniT0MQt+bdpw9AlWZfLIxz KBSK0RwskBXGHIolb2bE8hp289TTJu0J1WWzVvy53VzOKq8s9XEnRavky2KhgH4m4mK6 661Y9IQw7JoLypVlrdoBD8sxAwthxfaXKEmdCKtOGV3D/nNxr41Sp4bEdMUJj+P+OlXe eTtBfE1UOQLSMw7KAng+Tm+jfcY/uuVSs2tAap6Txhn+q2qjZQ+unpzTV8h8MNFyzlca Kodg== X-Gm-Message-State: AD7BkJK67ayOgZ/ll0JQfK5+znjh9gCHiHUaGQCTFFQbvw2dUMpQL0dbxTQAb7jDTpFLQ92HDDBZiySY7nEdUw== MIME-Version: 1.0 X-Received: by 10.28.126.4 with SMTP id z4mr26455559wmc.29.1458736913638; Wed, 23 Mar 2016 05:41:53 -0700 (PDT) Received: by 10.194.121.132 with HTTP; Wed, 23 Mar 2016 05:41:53 -0700 (PDT) In-Reply-To: <56F07CA0.5040607@redhat.com> References: <56E12FFF.6050800@t-online.de> <56E132FC.2030404@redhat.com> <1457734166.5425.43.camel@redhat.com> <56E6BAB1.6030804@redhat.com> <1458591323.9902.81.camel@redhat.com> <56F07CA0.5040607@redhat.com> Date: Wed, 23 Mar 2016 13:41:53 +0100 Message-ID: Subject: Re: Fix 69650, bogus line numbers from libcpp From: Richard Biener To: Bernd Schmidt Cc: David Malcolm , GCC Patches X-IsSubscribed: yes On Mon, Mar 21, 2016 at 11:58 PM, Bernd Schmidt wrote: > On 03/21/2016 09:15 PM, David Malcolm wrote: >> >> On Mon, 2016-03-14 at 14:20 +0100, Bernd Schmidt wrote: >>> >>> On 03/11/2016 11:09 PM, David Malcolm wrote: >>>>> >>>>> + cpp_error (pfile, CPP_DL_ERROR, >>>>> + "file \"%s\" left but not entered", >>>>> new_file); >>>> >>>> ^^^^^^^^ >>>> Although it looks like you're preserving the existing behavior from >>>> when this was in linemap_add, shouldn't this be >>>> ORDINARY_MAP_FILE_NAME (from) >>>> rather than new_file? (i.e. shouldn't it report the name of the >>>> file >>>> being *left*, rather than the one being entered?) >> >> >> I realize now that I was wrong here: "new_file" refers to the filename >> given in the linemarker directive, which, depending on the (optional) >> flag could be a directive to enter or leave a linemap. >> >> Sorry about that; you may want to disregard my earlier worries. > > > No, I think you were mostly on point: new_file is the one in the #line > directive, which AFAICT is the file being reentered. so the wording is in > fact misleading. Including a file called "t.h" from "v.c" produces this > pattern: > > # 1 "t.h" 1 > int t; > # 2 "v.c" 2 > >> I was thinking of something like the attached, which makes things more >> explicit; I felt the first dg-error in your patch was a little too >> concise. > > > No problem, but I do think we want to change the wording of the message to > something like "file %s unexpectedly reentered". > >> I'm very familiar with the code for tracking ranges and issuing >> diagnostics, but less familiar with other parts of libcpp, so I'm not >> sure I'm fully qualified to approve the patch. That said, the patch >> looks sane, mostly... The remaining thing I have a concern about >> relates to the other question I had, which I don't think you addressed: >> is it possible to construct a testcase that triggers the logic in the >> non-MAIN_FILE_P clause? > > > Are you thinking of anything more complex than the following? > > # 1 "v.c" > # 1 "t.h" 1 > # 1 "t2.h" 1 > int t; > # 2 "t.h" 2 > # 2 "v.c" 2 > > Change any of the filenames of the "^#.*2$" lines and you'll see error > messages. For example, changing "t.h" to "x.h" in the second to last line: > > In file included from t.h:1:0, > from v.c:1: > t2.h:2:13: error: file "x.h" left but not entered > t2.h:3:12: error: file "v.c" left but not entered > > Of course any such error is likely to have a large number of follow-on > errors. Not sure how to avoid this given that the input file most likely has > a completely messed up structure. > >> (How much existing test coverage do we have for line-markers? I found >> about 15 existing testcases that use them in a crude search with grep, >> but these are all apparently only incidentally as part of testing other >> functionality; is it worth me adding some more general test coverage >> for this?) > > > It might be worthwhile but I'm not planning to for this issue. Btw, the issue in the PR is also fixed with a simple I did some archeology and the revision that introduced the error || is 44789 which documented that part as (add_line_map): Return pointer to const. When passed NULL to_file with LC_LEAVE, use the obvious values for the return point so the caller doesn't have to figure them out. where obviously the values used in the error case are not obvious. Simply keeping the info parsed from the line directive makes us happy here. Bootstrap/test still running, I also have a LTO testcase for the crash. Note this doesn't make the testcase error as I think your patch does (which I think is an improvement in itself but possibly would require some -fpermissive handling as I expect some old code generators to generate invalid line directives). Richard. > > Bernd > Index: libcpp/line-map.c =================================================================== --- libcpp/line-map.c (revision 234415) +++ libcpp/line-map.c (working copy) @@ -543,7 +543,7 @@ linemap_add (struct line_maps *set, enum to_file); /* A TO_FILE of NULL is special - we use the natural values. */ - if (error || to_file == NULL) + if (to_file == NULL) { to_file = ORDINARY_MAP_FILE_NAME (from); to_line = SOURCE_LINE (from, from[1].start_location);