From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id C79973950422 for ; Wed, 3 Mar 2021 15:24:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C79973950422 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rguenther@suse.de X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 798E0AF9A; Wed, 3 Mar 2021 15:23:59 +0000 (UTC) Date: Wed, 3 Mar 2021 16:23:59 +0100 (CET) From: Richard Biener To: David Malcolm cc: Martin Sebor , Jeff Law , gcc-patches@gcc.gnu.org, Jakub Jelinek Subject: Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer In-Reply-To: <398db1f4fe828ac99258953266ca283ee75535e6.camel@redhat.com> Message-ID: References: <72ba6217-a6ec-c5dc-b3fe-977b0c9ce2b8@gmail.com> <398db1f4fe828ac99258953266ca283ee75535e6.camel@redhat.com> User-Agent: Alpine 2.21 (LSU 202 2017-01-01) MIME-Version: 1.0 X-Spam-Status: No, score=-5.4 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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: Wed, 03 Mar 2021 15:24:02 -0000 On Wed, 3 Mar 2021, David Malcolm wrote: > On Wed, 2021-03-03 at 08:48 +0100, Richard Biener wrote: > > On Tue, 2 Mar 2021, Martin Sebor wrote: > > > > > On 3/2/21 9:52 AM, Jeff Law via Gcc-patches wrote: > > > > > > > > > > > > On 3/1/21 1:39 AM, Richard Biener wrote: > > > > > The default diagnostic tree printer relies on dump_generic_node > > > > > which > > > > > for some reason manages to clobber the diagnostic pretty- > > > > > printer state > > > > > so we see garbled diagnostics like > > > > > > > > > > /home/rguenther/src/trunk/gcc/calls.c: In function > > > > > 'expand_call': > > > > > D.6750.coeffs[0]'/home/rguenther/src/trunk/gcc/dojump.c:118:28: > > > > > warning: > > > > > may be used uninitialized in this function [-Wmaybe- > > > > > uninitialized] > > > > > > > > > > when the diagnostic is emitted by the LTO fronted.  The > > > > > following > > > > > approach using a temporary pretty-printer for the > > > > > dump_generic_node > > > > > call fixes this for some unknown reason and we issue > > > > > > > > > > /home/rguenther/src/trunk/gcc/calls.c: In function > > > > > 'expand_call': > > > > > /home/rguenther/src/trunk/gcc/dojump.c:118:28: warning: > > > > > 'MEM[(struct > > > > > poly_int *)&save].D.6750.coeffs[0]' may be used uninitialized > > > > > in this > > > > > function [-Wmaybe-uninitialized] > > > > > > > > > > [LTO] Bootstrapped and tested on x86_64-unknown-linux-gnu, OK > > > > > for trunk? > > > > > > > > > > Thanks, > > > > > Richard. > > > > > > > > > > 2021-02-26  Richard Biener  > > > > > > > > > >  PR middle-end/97855 > > > > >  * tree-diagnostic.c (default_tree_printer): Use a temporary > > > > >  pretty-printer when formatting a tree via dump_generic_node. > > > > It'd be good to know why this helps, but I trust your judgment > > > > that this > > > > is an improvement. > > > > > > I don't know if it's related but pr98492 tracks a problem in the > > > C++ > > > front end caused by reinitializing the pretty printer in a number > > > of > > > functions in cp/error.c.  When one of these functions is called > > > while > > > the pretty printer is formatting something, the effect of > > > the reinitialization is to drop the already formatted contents > > > of the printer's buffer. > > > > > > IIRC, I tripped over this when working on the MEM_REF formatting > > > improvement for -Wuninitialized. > > > > I've poked quite a bit with breakpoints on suspicious pretty-printer > > functions and watch points on the pp state but found nothing in the > > case I was looking at (curiously also -Wuninitialized).  But I also > > wasn't able to understand why the caller should work at all.  And > > yes, the C/C++ tree printers also simply format to the passed > > pretty-printer... > > > > Hoping that David could shed some light on how this should play > > together. > > This looks very much like the issue I ran into in > c46d057f55748520e819dcd8e04bca71be9902b2 (and, in retrospect, that > commit may have just been papering over the problem). > > The issue there was that pp_printf is not reentrant - if a handler for > a pp_printf format code ends up making a nested call to pp_printf, I > got behavior that looks like what you're seeing. > > That said, I've been poring over the output in PR middle-end/97855 and > comparing it to the various pretty-printer usage in the tree, and I'm > not seeing anywhere where a pp_printf seems to be used when generating: > MEM[(struct poly_int *)&save + 8B].D.6750.coeffs[0] I think it's the D.6750 which is printed via else if (TREE_CODE (node) == DEBUG_EXPR_DECL) { if (flags & TDF_NOUID) pp_string (pp, "D#xxxx"); else pp_printf (pp, "D#%i", DEBUG_TEMP_UID (node)); because this is a DECL_DEBUG_EXPR. One could experiment with avoiding pp_printf in dump_decl_name. > Is there a minimal reproducer (or a .i file?) No, you need to do a LTO bootstrap, repeat the link step of for example cc1 with -v -save-temps and pick an ltrans invocation that exhibits the issue ... I can poke at the above tomorrow again. I suppose we could also add some checking-assert into the pp_printf code at the problematic place (or is any recursion bogus?) to catch the case with an ICE. Richard. > Dave > > > > >   Most specifically > > > >   pp_format (context->printer, &diagnostic->message); > > > > ^^^ this is the path affected by the patch > > > >   (*diagnostic_starter (context)) (context, diagnostic); > > > > ^^^ this somehow messes things up, it does pp_set_prefix on > > context->printer but also does some formatting > > > >   pp_output_formatted_text (context->printer); > > > > and now we expect this to magically output the composed pieces. > > > > Note swapping the first two lines didn't have any effect (I don't > > remember if it changed anything so details might have changed but > > it was definitely still broken). > > > > That said, the only hint I have is that the diagnostic plus prefix > > is quite long, but any problem in the generic code should eventually > > affect non-LTO as well but the PR is reported for LTO only > > (bogus diagnostics shown during LTO bootstrap).  The patch fixes > > all bogus diagnostics during LTO bootstrap. > > > > I originally thought there's maybe a pp_flush too much but maybe > > there's a pp_flush missing ... > > > > I'll wait for Davids feedback but will eventually install the > > patch to close the bug. > > > > Richard. > > > > > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)