From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id B7F0F396ECB0 for ; Thu, 1 Jul 2021 15:41:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B7F0F396ECB0 Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-30-wQF7e0IkPjGnrXhUyJOnfA-1; Thu, 01 Jul 2021 11:40:59 -0400 X-MC-Unique: wQF7e0IkPjGnrXhUyJOnfA-1 Received: by mail-qv1-f72.google.com with SMTP id p12-20020a0cfacc0000b02902a1b4396bc4so2081279qvo.22 for ; Thu, 01 Jul 2021 08:40:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=5ifVhd/T5BHm2kMmi8ibsys+FuTjYKDJ8wkx2VvbHTU=; b=TQXv8cCEtZxj56K2h26OU9cprCeJGLzn1yhwfjZi5bRu/7HbqmesYaWpY2Sq2WKfjB 65zT5jc/pqxs28favYmms27WJguwcUX3dJ6DCBD4kR4rGffnxTPOzXvRWVnB3MzMeSpW Lilmx2pYW36fEIqVG2Qj8X0Iwd1smDFAcPxC8/m9GU4K8w/hNNx5jFOE9FhVYL0yFXm2 JSLVR9DyggqCwz8dZXUCRLEFZJ0wSfkfT+M6wdosCmSzanrfrlYHZ5wJf/0Lta/+defq PuRyUVs0VVeDiguQPwfi73+a/95aBbqdVliyLLcflrjFmDexxAJM7DBn/1GPDBGoE7zE bezg== X-Gm-Message-State: AOAM532p3sOAh51bk6VXWoAXYn6MiUop0FrX6PibTObuA0WsclUToY0f 0JBX/+v23BKOMhQ2nt+4qLQf57gFXaolPvUvK5VFN0Dym+7C8aJlTLQd5dFuJRU9isAxki9uIy7 m8KVaOSp9cv9rWIRcFg== X-Received: by 2002:a05:622a:13d2:: with SMTP id p18mr564937qtk.224.1625154058665; Thu, 01 Jul 2021 08:40:58 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyCV544QkNoghvr+EieMKGkAPW+0PZ+1mVPODZ96udmN/XGmMUPBfPZd/fm8e2Dn4l5emkDNw== X-Received: by 2002:a05:622a:13d2:: with SMTP id p18mr564929qtk.224.1625154058449; Thu, 01 Jul 2021 08:40:58 -0700 (PDT) Received: from t14s.localdomain (c-73-69-212-193.hsd1.ma.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id x125sm100164qkd.8.2021.07.01.08.40.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 01 Jul 2021 08:40:56 -0700 (PDT) Message-ID: <1d9c0179fa3e19f9056ac65fd05964866315e2c5.camel@redhat.com> Subject: Re: [PATCH 2/4] allow poisoning input_location in ranges it should not be used From: David Malcolm To: Richard Biener , Trevor Saunders Cc: GCC Patches Date: Thu, 01 Jul 2021 11:40:55 -0400 In-Reply-To: References: <20210630053529.26581-1-tbsaunde@tbsaunde.org> <20210630053529.26581-2-tbsaunde@tbsaunde.org> <907daf69d72fedce3dd9ee8a9dccc59d7d22a08a.camel@redhat.com> User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Thu, 01 Jul 2021 15:41:02 -0000 On Thu, 2021-07-01 at 14:53 +0200, Richard Biener wrote: > On Thu, Jul 1, 2021 at 12:16 PM Trevor Saunders < > tbsaunde@tbsaunde.org> wrote: > > > > On Wed, Jun 30, 2021 at 11:13:23AM -0400, David Malcolm wrote: > > > On Wed, 2021-06-30 at 01:35 -0400, Trevor Saunders wrote: > > > > This makes it possible to assert if input_location is used > > > > during the > > > > lifetime > > > > of a scope.  This will allow us to find places that currently > > > > use it > > > > within a > > > > function and its callees, or prevent adding uses within the > > > > lifetime > > > > of a > > > > function after all existing uses are removed. > > > > > > > > bootstrapped and regtested on x86_64-linux-gnu, ok? > > > > > > > > Trev > > > > > > [...snip...] > > > > > > > diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c > > > > index d58586f2526..3f68d1d79eb 100644 > > > > --- a/gcc/diagnostic.c > > > > +++ b/gcc/diagnostic.c > > > > @@ -1835,7 +1835,7 @@ internal_error (const char *gmsgid, ...) > > > >    auto_diagnostic_group d; > > > >    va_list ap; > > > >    va_start (ap, gmsgid); > > > > -  rich_location richloc (line_table, input_location); > > > > +  rich_location richloc (line_table, UNKNOWN_LOCATION); > > > >    diagnostic_impl (&richloc, NULL, -1, gmsgid, &ap, DK_ICE); > > > >    va_end (ap); > > > > > > > > > > I actually make use of this in the analyzer: the analyzer sets > > > input_location to stmt->location when analyzing a given stmt - > > > that > > > way, if the analyzer ICEs, the ICE is shown at the code construct > > > that > > > crashed the analyzer. > > > > > > This behavior is useful to me, and would be lost with the > > > proposed > > > patch. > > > > I made this change because otherwise if the compiler ICE's while > > access > > to input_location is blocked we end up infinitely recursing > > complaining > > we can't access it while trying to say where the last error was.  I > > was > > nervous about the change before, and now I agree we need something > > else. > > > > > Is there a better way of doing what I'm doing? > > > > > > Is the long-term goal of the patch kit to reduce our reliance on > > > global > > > variables?  Are we ultimately still going to need a variable for > > > "where > > > to show the ICE if gcc crashes"?  (perhaps stashing it in the > > > diagnostic_context???) > > > > Yes, the goal is ultimately removal of global state, however I'm > > not > > really ure what the better approach to your problem is, after all > > even > > moving it to the diagnostic context is sort of a global state, and > > sort > > of dupplicates input_location.  That said it is somewhat more > > constrained, so if it removes usage of input_location perhaps its > > worthwhile? > > Reduction of global state is of course good - but in particular > input_location > should be something only used during parsing because it's a quite > broken concept otherwise.  And fiddling with it tends to be quite > fragile... > for example see g:7d6f7e92c3b737736a2d8ff97a71af9f230c2f88 > for the "fun" you can have with "stale" values in input_location ... Yeah. Another example, from the analyzer, is g:2fbea4190e76a59c4880727cf84706fe083c00ae (PR 93349) > IMHO users should have their own "copy", for example the gimplifier > instead of mucking with and using input_location could use a > similar state in its gimplify_ctx. Some ideas (not necessarily good ones): (a) the diagnostic_context could have an ice_location field, and use that in internal_error (and maybe an RAII class for setting/clearing it). (b) move input_location to diagnostic_context, and add: #define input_location (global_dc->x_input_location) or: #define input_location (global_dc->x_default_location) which add an indirection everywhere. I don't love these ideas, in that we already overuse the preprocessor IMHO. Trevor: BTW, if you're looking for global state to eliminate, it might be nice to move the globals in input.c for caching source lines (fcache_tab etc) into a new source_cache class, and have the diagnostic_context own it via a new "source_cache *" field. Dave