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 9D5183857419 for ; Thu, 1 Jul 2021 16:04:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9D5183857419 Received: from mail-qk1-f200.google.com (mail-qk1-f200.google.com [209.85.222.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-248-PdmTRSNKNmmG6SJEOz9pZw-1; Thu, 01 Jul 2021 12:04:27 -0400 X-MC-Unique: PdmTRSNKNmmG6SJEOz9pZw-1 Received: by mail-qk1-f200.google.com with SMTP id i3-20020a05620a1503b02903b2ffa0a87fso4551465qkk.18 for ; Thu, 01 Jul 2021 09:04:27 -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=b/gYqKDm0PkCXfipat8tyPkn2+D2tkMaOO+5KdmbsI4=; b=STmb1aYF10fpzdjFN/PKdf5ZNPf8jOPr5Saz0Op7yUAviSfSfjZFXnxa5M8SbWnS1p nqFEAECj1yaeIRPtn2YjWY6mDhrL51rFiOEq1SrMj5CIQYojkjKJk9dqHhuD1jKM/tCd /Za84XNRjeR4KLqB/hN7Rh+y9SzljFLm3uIlxUdD21HHyBGYehPDmVip2Wg+Qew8Y4/b wTYLsfvUcg9dVn1L3CJfjRPTWa2pZvVJFqFIKqNU6HFKIIwAig5zV+stoV24hSgpMYIf X8IzdWsyYFm8kpmEZlheBsT2/Xf6UBWIMWXnrz6U9y6S1hToM4M8FO/sZ+2HHX9ta/1B AbgA== X-Gm-Message-State: AOAM532XxuH710gcNqDxAP+Xw43SsDeqEa6HtQLiKh2zG2FtmA4Txfzx WFXyN7COt+avZOmny7vd7g3v0EfcGq+JRIVeMVIzwQgxIVLYIWpBfYevaLqSS9OaWbCezFP9ghf Bty0snQYr4CdzsBmejg== X-Received: by 2002:ae9:ddc4:: with SMTP id r187mr784817qkf.198.1625155466900; Thu, 01 Jul 2021 09:04:26 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxJeRgESa/r06fRFVyX7+ktA5Y/RAnUOtxIt2iLb2V/g6OtNe6ISOMd7sEfBaThBamtxdsHHg== X-Received: by 2002:ae9:ddc4:: with SMTP id r187mr784796qkf.198.1625155466709; Thu, 01 Jul 2021 09:04:26 -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 l9sm141929qtk.51.2021.07.01.09.04.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 01 Jul 2021 09:04:26 -0700 (PDT) Message-ID: <53b40be4f996970ca52ac6fc05419683c2cbfa1d.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 12:04:25 -0400 In-Reply-To: <1d9c0179fa3e19f9056ac65fd05964866315e2c5.camel@redhat.com> References: <20210630053529.26581-1-tbsaunde@tbsaunde.org> <20210630053529.26581-2-tbsaunde@tbsaunde.org> <907daf69d72fedce3dd9ee8a9dccc59d7d22a08a.camel@redhat.com> <1d9c0179fa3e19f9056ac65fd05964866315e2c5.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=-10.4 required=5.0 tests=BAYES_00, BODY_8BITS, 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 16:04:30 -0000 On Thu, 2021-07-01 at 11:40 -0400, David Malcolm wrote: > 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. Actually, I just found an old working copy on my drive that does most of this; I'll try cleaning it up for gcc 12 and see if I can get it to bootstrap. Dave