From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x131.google.com (mail-lf1-x131.google.com [IPv6:2a00:1450:4864:20::131]) by sourceware.org (Postfix) with ESMTPS id 98FEF3858C52 for ; Wed, 5 Jun 2024 17:07:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 98FEF3858C52 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 98FEF3858C52 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::131 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1717607237; cv=none; b=CoKgdv56BOlxQBqupmpntAM8YXj9eQu78nbLrnrEKITC2YfNBluWsZOT+IuS1zgrF6jl1iuxKwijyE1mQSxTg/6NLadBl1yXwGJqjM6N7W4bqbvDp/D8LY2FPg4MauM2d2hJiC3c9KGmL3nuefWZ5RcKh1IlF7DHtXxBZSOLjMk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1717607237; c=relaxed/simple; bh=zVSaNotQ7TxgA1HJGQrT+2dSZEuAC4/2vCpd74EdaPc=; h=DKIM-Signature:From:Mime-Version:Subject:Date:Message-Id:To; b=RH+uCtW10UbPh6ZIW1jQr1g+jG+6BuwSfEm8JOwOrpG1XhHqgogdqS0+s+LXsb+D3OxZZOzY81AqJ+wP0RnKesH09Ud53aMp4uCy3/KmEli4B+pQ+QDtLdZ+h6fTYfWv5xALtYGpYHw5f5W25oJ6oG6D/5oJC3xiBzXpO9CRCIQ= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-lf1-x131.google.com with SMTP id 2adb3069b0e04-5295eb47b48so103038e87.1 for ; Wed, 05 Jun 2024 10:07:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1717607232; x=1718212032; darn=gcc.gnu.org; h=to:in-reply-to:cc:references:message-id:date:subject:mime-version :from:content-transfer-encoding:from:to:cc:subject:date:message-id :reply-to; bh=x9loqu4wB5EXSvpjrgRKvxLTytmuHoTBOiPki/+VgHE=; b=S6AGej+6uNvfpe9F30SF40dxg+MWBm3khNYIwG3Im0irlKsw+L1uPg59GUnDDFMlYy RnlsHafsskyVwXV4jpgdy4h2FQpSJkQq9/ZP1NgomOUNkh9nKIr2T77BYmB7QP5YVEZp 6w+BkW3IOQFkYcEq9oKgUCqxSK1wg8W7IT3Po2CwdpsRXf515iJghNIgo+1Cbz5KpqFx 8qM9XHzp4R+r3t/SGHspjID4/pvkQMs5dvF2oeaGLpPE9DsfbiZHR+ZHqxIjYgLTGOnH L6zdsNQpTnhHVocHt+4n37Ci9ls5g8qk0+qYtTdb86QAsXbMwq3Xw62LGWTIkExdkKWg uuMA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717607232; x=1718212032; h=to:in-reply-to:cc:references:message-id:date:subject:mime-version :from:content-transfer-encoding:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=x9loqu4wB5EXSvpjrgRKvxLTytmuHoTBOiPki/+VgHE=; b=n2fT6MwdClGR17YpsAVqRVucrr3oI2MOqueJJa7rZ2qOHBe1g65+DfNsSGgPaJoT12 Vakpe9jkXZcn88aBzgg9d4jg06mEkuj4UAkUZ8HF3TDslMYZMTd8uK3QY/9tbPFf81OY Q6vDvSZ/LUXJ4Q360kwXydyvGBflx4yiHZz9ryPu95yOhis9w2SJiJB4Bz1N0/NaTQQL dg2QPQN7Kn8dvSCqpcLTJhVEE2r0d3xgvYBnExqXDiUNCsBY8Gjju89SjjcMSQRWbIn7 72eoha9mn3jjTvy7TtTOuf00azemcTa5XG0x0ZH3MxcRYveHCFfCGHc6PspR6YKDzdt8 2GNA== X-Forwarded-Encrypted: i=1; AJvYcCXfsIJV5ZOehOXPlRU+gB2+yuEFzF5UatPeAVusYXHGQsk6vNlY0xt6N0h0lD3ssJ2wfSJH2nh2v0MXFsyVUknMlaaadYNOng== X-Gm-Message-State: AOJu0YxEVh5Ny+j7nn6815Kf1oZ3nXq1BsPPh1J4g7hZmmYUO1CO/P+9 5CExfJFFTj7mWbjzU438VYXZBGmfvcAK3SPxPairIe+gjvdEUU6K X-Google-Smtp-Source: AGHT+IHV+dj0I97HmDdJHaxieORrT0q3o/BVmJw7xTMiCV2ynEo1HUN3WKJgDEKjw10scnaVCcpKhg== X-Received: by 2002:ac2:44d4:0:b0:52a:d87f:60d7 with SMTP id 2adb3069b0e04-52bab4fbe43mr1941498e87.46.1717607231449; Wed, 05 Jun 2024 10:07:11 -0700 (PDT) Received: from smtpclient.apple (dynamic-077-009-068-211.77.9.pool.telefonica.de. [77.9.68.211]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-57a31b98e0csm9465248a12.13.2024.06.05.10.07.10 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 05 Jun 2024 10:07:11 -0700 (PDT) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable From: Richard Biener Mime-Version: 1.0 (1.0) Subject: Re: [RFC][PATCH] PR tree-optimization/109071 - -Warray-bounds false positive warnings due to code duplication from jump threading Date: Wed, 5 Jun 2024 19:07:00 +0200 Message-Id: References: <7966EE0D-6EA5-493B-846A-EAB4A4087B3A@oracle.com> Cc: David Malcolm , GCC Patches , Andrew Pinski , kees Cook In-Reply-To: <7966EE0D-6EA5-493B-846A-EAB4A4087B3A@oracle.com> To: Qing Zhao X-Mailer: iPhone Mail (21F90) X-Spam-Status: No, score=-10.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: > Am 05.06.2024 um 18:39 schrieb Qing Zhao : >=20 > =EF=BB=BF >=20 >> On Jun 5, 2024, at 03:26, Richard Biener wro= te: >>=20 >>> On Tue, Jun 4, 2024 at 10:31=E2=80=AFPM Qing Zhao = wrote: >>>=20 >>>=20 >>>=20 >>>> On Jun 4, 2024, at 03:43, Richard Biener w= rote: >>>>=20 >>>> On Mon, Jun 3, 2024 at 4:48=E2=80=AFPM David Malcolm wrote: >>>>>=20 >>>>> On Mon, 2024-06-03 at 08:29 +0200, Richard Biener wrote: >>>>>> On Fri, May 31, 2024 at 11:23=E2=80=AFPM Qing Zhao >>>>>> wrote: >>>>>>>=20 >>>>>>>=20 >>>>>>>=20 >>>>>>>> On May 23, 2024, at 07:46, Richard Biener >>>>>>>> wrote: >>>>>>>>=20 >>>>>>>> On Wed, May 22, 2024 at 8:53=E2=80=AFPM Qing Zhao >>>>>>>> wrote: >>>>>>>>>=20 >>>>>>>>>=20 >>>>>>>>>=20 >>>>>>>>>> On May 22, 2024, at 03:38, Richard Biener >>>>>>>>>> wrote: >>>>>>>>>>=20 >>>>>>>>>> On Tue, May 21, 2024 at 11:36=E2=80=AFPM David Malcolm >>>>>>>>>> wrote: >>>>>>>>>>>=20 >>>>>>>>>>> On Tue, 2024-05-21 at 15:13 +0000, Qing Zhao wrote: >>>>>>>>>>>> Thanks for the comments and suggestions. >>>>>>>>>>>>=20 >>>>>>>>>>>>> On May 15, 2024, at 10:00, David Malcolm >>>>>>>>>>>>> >>>>>>>>>>>>> wrote: >>>>>>>>>>>>>=20 >>>>>>>>>>>>> On Tue, 2024-05-14 at 15:08 +0200, Richard Biener >>>>>>>>>>>>> wrote: >>>>>>>>>>>>>> On Mon, 13 May 2024, Qing Zhao wrote: >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> -Warray-bounds is an important option to enable >>>>>>>>>>>>>>> linux kernal to >>>>>>>>>>>>>>> keep >>>>>>>>>>>>>>> the array out-of-bound errors out of the source >>>>>>>>>>>>>>> tree. >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> However, due to the false positive warnings >>>>>>>>>>>>>>> reported in >>>>>>>>>>>>>>> PR109071 >>>>>>>>>>>>>>> (-Warray-bounds false positive warnings due to code >>>>>>>>>>>>>>> duplication >>>>>>>>>>>>>>> from >>>>>>>>>>>>>>> jump threading), -Warray-bounds=3D1 cannot be added >>>>>>>>>>>>>>> on by >>>>>>>>>>>>>>> default. >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> Although it's impossible to elinimate all the false >>>>>>>>>>>>>>> positive >>>>>>>>>>>>>>> warnings >>>>>>>>>>>>>>> from -Warray-bounds=3D1 (See PR104355 Misleading - >>>>>>>>>>>>>>> Warray-bounds >>>>>>>>>>>>>>> documentation says "always out of bounds"), we >>>>>>>>>>>>>>> should minimize >>>>>>>>>>>>>>> the >>>>>>>>>>>>>>> false positive warnings in -Warray-bounds=3D1. >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> The root reason for the false positive warnings >>>>>>>>>>>>>>> reported in >>>>>>>>>>>>>>> PR109071 is: >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> When the thread jump optimization tries to reduce >>>>>>>>>>>>>>> the # of >>>>>>>>>>>>>>> branches >>>>>>>>>>>>>>> inside the routine, sometimes it needs to duplicate >>>>>>>>>>>>>>> the code >>>>>>>>>>>>>>> and >>>>>>>>>>>>>>> split into two conditional pathes. for example: >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> The original code: >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> void sparx5_set (int * ptr, struct nums * sg, int >>>>>>>>>>>>>>> index) >>>>>>>>>>>>>>> { >>>>>>>>>>>>>>> if (index >=3D 4) >>>>>>>>>>>>>>> warn (); >>>>>>>>>>>>>>> *ptr =3D 0; >>>>>>>>>>>>>>> *val =3D sg->vals[index]; >>>>>>>>>>>>>>> if (index >=3D 4) >>>>>>>>>>>>>>> warn (); >>>>>>>>>>>>>>> *ptr =3D *val; >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> return; >>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> With the thread jump, the above becomes: >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> void sparx5_set (int * ptr, struct nums * sg, int >>>>>>>>>>>>>>> index) >>>>>>>>>>>>>>> { >>>>>>>>>>>>>>> if (index >=3D 4) >>>>>>>>>>>>>>> { >>>>>>>>>>>>>>> warn (); >>>>>>>>>>>>>>> *ptr =3D 0; // Code duplications since >>>>>>>>>>>>>>> "warn" does >>>>>>>>>>>>>>> return; >>>>>>>>>>>>>>> *val =3D sg->vals[index]; // same this line. >>>>>>>>>>>>>>> // In this path, >>>>>>>>>>>>>>> since it's >>>>>>>>>>>>>>> under >>>>>>>>>>>>>>> the condition >>>>>>>>>>>>>>> // "index >=3D 4", the >>>>>>>>>>>>>>> compiler >>>>>>>>>>>>>>> knows >>>>>>>>>>>>>>> the value >>>>>>>>>>>>>>> // of "index" is >>>>>>>>>>>>>>> larger then 4, >>>>>>>>>>>>>>> therefore the >>>>>>>>>>>>>>> // out-of-bound >>>>>>>>>>>>>>> warning. >>>>>>>>>>>>>>> warn (); >>>>>>>>>>>>>>> } >>>>>>>>>>>>>>> else >>>>>>>>>>>>>>> { >>>>>>>>>>>>>>> *ptr =3D 0; >>>>>>>>>>>>>>> *val =3D sg->vals[index]; >>>>>>>>>>>>>>> } >>>>>>>>>>>>>>> *ptr =3D *val; >>>>>>>>>>>>>>> return; >>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> We can see, after the thread jump optimization, the >>>>>>>>>>>>>>> # of >>>>>>>>>>>>>>> branches >>>>>>>>>>>>>>> inside >>>>>>>>>>>>>>> the routine "sparx5_set" is reduced from 2 to 1, >>>>>>>>>>>>>>> however, due >>>>>>>>>>>>>>> to >>>>>>>>>>>>>>> the >>>>>>>>>>>>>>> code duplication (which is needed for the >>>>>>>>>>>>>>> correctness of the >>>>>>>>>>>>>>> code), >>>>>>>>>>>>>>> we >>>>>>>>>>>>>>> got a false positive out-of-bound warning. >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> In order to eliminate such false positive out-of- >>>>>>>>>>>>>>> bound warning, >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> A. Add one more flag for GIMPLE: is_splitted. >>>>>>>>>>>>>>> B. During the thread jump optimization, when the >>>>>>>>>>>>>>> basic blocks >>>>>>>>>>>>>>> are >>>>>>>>>>>>>>> duplicated, mark all the STMTs inside the original >>>>>>>>>>>>>>> and >>>>>>>>>>>>>>> duplicated >>>>>>>>>>>>>>> basic blocks as "is_splitted"; >>>>>>>>>>>>>>> C. Inside the array bound checker, add the >>>>>>>>>>>>>>> following new >>>>>>>>>>>>>>> heuristic: >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> If >>>>>>>>>>>>>>> 1. the stmt is duplicated and splitted into two >>>>>>>>>>>>>>> conditional >>>>>>>>>>>>>>> paths; >>>>>>>>>>>>>>> + 2. the warning level < 2; >>>>>>>>>>>>>>> + 3. the current block is not dominating the exit >>>>>>>>>>>>>>> block >>>>>>>>>>>>>>> Then not report the warning. >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> The false positive warnings are moved from -Warray- >>>>>>>>>>>>>>> bounds=3D1 to >>>>>>>>>>>>>>> -Warray-bounds=3D2 now. >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> Bootstrapped and regression tested on both x86 and >>>>>>>>>>>>>>> aarch64. >>>>>>>>>>>>>>> adjusted >>>>>>>>>>>>>>> -Warray-bounds-61.c due to the false positive >>>>>>>>>>>>>>> warnings. >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> Let me know if you have any comments and >>>>>>>>>>>>>>> suggestions. >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> At the last Cauldron I talked with David Malcolm >>>>>>>>>>>>>> about these kind >>>>>>>>>>>>>> of >>>>>>>>>>>>>> issues and thought of instead of suppressing >>>>>>>>>>>>>> diagnostics to >>>>>>>>>>>>>> record >>>>>>>>>>>>>> how a block was duplicated. For jump threading my >>>>>>>>>>>>>> idea was to >>>>>>>>>>>>>> record >>>>>>>>>>>>>> the condition that was proved true when entering the >>>>>>>>>>>>>> path and do >>>>>>>>>>>>>> this >>>>>>>>>>>>>> by recording the corresponding locations >>>>>>>>>>>>=20 >>>>>>>>>>>> Is only recording the location for the TRUE path enough? >>>>>>>>>>>> We might need to record the corresponding locations for >>>>>>>>>>>> both TRUE and >>>>>>>>>>>> FALSE paths since the VRP might be more accurate on both >>>>>>>>>>>> paths. >>>>>>>>>>>> Is only recording the location is enough? >>>>>>>>>>>> Do we need to record the pointer to the original >>>>>>>>>>>> condition stmt? >>>>>>>>>>>=20 >>>>>>>>>>> Just to be clear: I don't plan to work on this myself (I >>>>>>>>>>> have far too >>>>>>>>>>> much already to work on...); I'm assuming Richard Biener is >>>>>>>>>>> hoping/planning to implement this himself. >>>>>>>>>>=20 >>>>>>>>>> While I think some of this might be an improvement to those >>>>>>>>>> vast >>>>>>>>>> number of "false positive" diagnostics we have from (too) >>>>>>>>>> late diagnostic >>>>>>>>>> passes I do not have the cycles to work on this. >>>>>>>>>=20 >>>>>>>>> I can study a little bit more on how to improve the diagnostics >>>>>>>>> for PR 109071 first. >>>>>>>>>=20 >>>>>>>>> FYI, I just updated PR109071=E2=80=99s description as: Need more >>>>>>>>> context for -Warray-bounds warnings due to code duplication >>>>>>>>> from jump threading. >>>>>>>>>=20 >>>>>>>>> As we already agreed, this is NOT a false positive. It caught a >>>>>>>>> real bug in linux kernel that need to be patched in the kernel >>>>>>>>> source. (See >>>>>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D109071#c11) >>>>>>>>>=20 >>>>>>>>> In order to add more context to the diagnostics to help the end >>>>>>>>> user locate the bug accurately in their source code, compiler >>>>>>>>> needs: >>>>>>>>>=20 >>>>>>>>> 1. During jump threading phase, record the following >>>>>>>>> information for each duplicated STMT: >>>>>>>>> A. A pointer to the COND stmt; >>>>>>>>> B. True/false for such COND >>>>>>>>> 2. During array out-of-bound checking phase, whenever locate an >>>>>>>>> out-of-bound case, >>>>>>>>> A. Use a rich_location for the diagnostic; >>>>>>>>> B. Create an instance of diagnositic_path, add events to >>>>>>>>> this diagnostic_path based on the COND stmt, True/False info >>>>>>>>> recorded in the STMT; >>>>>>>>> C. Call richloc.set_path() to associate the path with >>>>>>>>> the rich_location; >>>>>>>>> D. Then issue warning with this rich_location instead of >>>>>>>>> the current regular location. >>>>>>>>>=20 >>>>>>>>> Any comment and suggestion to the above? >>>>>>>>=20 >>>>>>>> I was originally thinking of using location_adhoc_data to store >>>>>>>> 1.A >>>>>>>> and 1.B as a common bit to associate to each >>>>>>>> copied stmt. IIRC location_adhoc_data->data is the stmts >>>>>>>> associated >>>>>>>> lexical block so we'd need to stuff another >>>>>>>> 'data' field into this struct, like a "copy history" where we >>>>>>>> could >>>>>>>> then even record copied-of-copy-of-X. >>>>>>>> locataion_adhoc_data seems imperfectly packed right now, with a >>>>>>>> 32bit >>>>>>>> hole before 'data' and 32bit unused >>>>>>>> at its end, so we might get away without memory use by re- >>>>>>>> ordering >>>>>>>> discriminator before 'data' ... >>>>>>>=20 >>>>>>> Like this? >>>>>>>=20 >>>>>>> diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h >>>>>>> index e6e2b0897572..ee344f91333b 100644 >>>>>>> --- a/libcpp/include/line-map.h >>>>>>> +++ b/libcpp/include/line-map.h >>>>>>> @@ -761,8 +761,9 @@ struct GTY(()) maps_info_macro { >>>>>>> struct GTY(()) location_adhoc_data { >>>>>>> location_t locus; >>>>>>> source_range src_range; >>>>>>> - void * GTY((skip)) data; >>>>>>> unsigned discriminator; >>>>>>> + void * GTY((skip)) data; >>>>>>> + void * GTY((skip)) copy_history; >>>>>>> }; >>>>>>> struct htab; >>>>>>=20 >>>>>> Yes. >>>>>>=20 >>>>>>> How about the copy_history? Do we need a new data structure (like >>>>>>> the following, or other suggestion) for this? Where should I add >>>>>>> this new data structure? >>>>>>=20 >>>>>> As it needs to be managed by libcpp it should be in this very same >>>>>> file. >>>>>>=20 >>>>>>> struct copy_history { >>>>>>> location_t condition; >>>>>>> Bool is_true_path; >>>>>>> } >>>>>>=20 >>>>>> I think we want a pointer to the previous copy-of state as well in >>>>>> case a stmt >>>>>> was copied twice. We'll see whether a single (condition) location >>>>>> plus edge flag >>>>>> is sufficient. I'd say we should plan for an enum to indicate the >>>>>> duplication >>>>>> reason at least (jump threading, unswitching, unrolling come to my >>>>>> mind). For >>>>>> jump threading being able to say "when is true/false" is >>>>>> probably >>>>>> good enough, though it might not always be easy to identify a single >>>>>> condition >>>>>> here given a threading path starts at an incoming edge to a CFG merge= >>>>>> and >>>>>> will usually end with the outgoing edge of a condition that we can >>>>>> statically >>>>>> evaluate. The condition controlling the path entry might not be >>>>>> determined >>>>>> fully by a single condition location. >>>>>>=20 >>>>>> Possibly building a full "diagnostic path" object at threading time >>>>>> might be >>>>>> the only way to record all the facts, but that's also going to be >>>>>> more >>>>>> expensive. >>>>>=20 >>>>> Note that a diagnostic_path represents a path through some kind of >>>>> graph, whereas it sounds like you want to be storing the *nodes* in th= e >>>>> graph, and later generating the diagnostic_path from that graph when w= e >>>>> need it (which is trivial if the graph is actually just a tree: just >>>>> follow the parent links backwards, then reverse it). >>>>=20 >>>> I think we are mixing two things - one is that a single transform like j= ump >>>> threading produces a stmt copy and when we emit a diagnostic on that >>>> copied statement we want to tell the user the condition under which the= >>>> copy is executed. That "condition" can be actually a sequence of >>>> conditionals. I wanted to point out that a diagnostic_path instance co= uld >>>> be used to describe such complex condition. >>>>=20 >>>> But then the other thing I wanted to address with the link to a previou= s >>>> copy_history - that's when a statement gets copied twice, for example >>>> by two distinct jump threading optimizations. Like when dumping >>>> the inlining decisions for diagnostics we could dump the logical "and" >>>> of the conditions of the two threadings. Since we have a single >>>> location per GIMPLE stmt we'd have to keep a "stack" of copy events >>>> associated with it. That's the linked list (I think a linked list shou= ld >>>> work fine here). >>> Yes, the linked list to keep the =E2=80=9Cstack=E2=80=9D of copy events s= hould be good enough >>> to form the sequence of conditionals event for the diagnostic_path insta= nce. >>>>=20 >>>> I realize things may get a bit "fat", but eventually we are not duplica= ting >>>> statements that much. I do hope we can share for example a big >>>> diagnostic_path when we duplicate a basic block during threading >>>> and use one instance for all stmts in such block copy (IIRC we never >>>> release locations or their ad-hoc data, we just make sure to never >>>> use locations with ad-hoc data pointing to BLOCKs that we released, >>>> but the linemap data will still have pointers in "dead" location entrie= s, >>>> right?) >>> Are you still suggesting to add two artificial stmts in the beginning an= d the >>> end of the duplicated block to carry the copy history information for al= l the >>> stmts in the block to save space? >>>=20 >>> Compared with the approach to carry such information to each duplicated s= tmts (which I preferred), >>> The major concerns with the approach are: >>> 1. Optimizations might move stmts out of these two artificial stmts, th= erefore we need add >>> Some memory barrier on these two artificial stmts to prevent such m= ovements. >>> This might prevent good optimization from happening and result in r= untime performance loss; >>> 2. Every time when query whether the stmt is copied and get its copy h= istory, we have to search backward or >>> Forward through the stmt chain to get to the artificial stmts that= carry the copy history, compilation time will >>> be slower. >>>=20 >>> I still think that carrying the copy history info to each duplicated stm= ts might be better. We can limit the length of >>> the history to control the space, what do you think? >>=20 >> Copying to each stmt is definitely easier so I think it's better to >> start with that and only resort >> to other things when this fails. > Okay. Will try this approach first. >>=20 >> Note we could, instead of putting a pointer to data into the ad-hoc >> info, put in an index >> and have the actual data be maintained elsewhere. That data could >> even only keep >> the "last" 1000 contexts and simply discard "older" ones. Before IPA >> info can still be >> transfered between functions, but after IPA where most of the >> threadings happen the >> extra info could be discarded once we get a function to the last pass >> emitting diagnostics >> we want to amend. Doing an index lookup makes it possible to not >> worry about "stale" >> entries. >=20 > Are you suggesting to use a global array with upper-bound =E2=80=9C1000=E2= =80=9D to hold the copy-history info? > Then for each copied stmt, put in an index to this array to refer the copy= -history linked list for it? > Is =E2=80=9C1000=E2=80=9D enough for one function? We can do some experime= nts to decide this number. >=20 > What do you mean by =E2=80=9Cstale=E2=80=9D entries? The entries for the p= revious function? >=20 > Is a function-level array enough for this purpose? i.e, for each function,= we use an array to hold the copy-history info, and free this array after th= is function is done with compilation? > A little confused here=E2=80=A6. I was thinking of a hash map and noting when we create an entry after IPA so= we can scrap it when done with the function=20 > Thanks for clarification. >=20 > Qing >=20 >=20 >>=20 >> Richard. >>=20 >>>=20 >>> Qing >>>=20 >>>>=20 >>>> Richard. >>>>=20 >>>>>=20 >>>>> Dave >>>>>=20 >>>>>> I can think of having a -fdiagnostic-try-to-explain-harder option to >>>>>> clarify confusing >>>>>> diagnostics people could use on-demand? >>>>>>=20 >>>>>> Richard. >>>>>>=20 >>>>>>> Qing >>>>>>>>=20 >>>>>>>> Richard. >>>>>>>>=20 >>>>>>>>> Thanks. >>>>>>>>>=20 >>>>>>>>> Qing >>>>>>>>>=20 >>>>>>>>>=20 >>>>>>>>>>=20 >>>>>>>>>> Richard. >>>>>>>>>>=20 >>>>>>>>>>> But feel free to ask me any questions about the >>>>>>>>>>> diagnostic_path >>>>>>>>>>> machinery within the diagnostics subsystem. >>>>>>>>>>>=20 >>>>>>>>>>> [...snip...] >>>>>>>>>>>=20 >>>>>>>>>>> Regards >>>>>>>>>>> Dave >=20 >=20