On Fri, Jun 27, 2025 at 3:39 PM Qing Zhao <qing.z...@oracle.com> wrote:
>
> Hi,
>
> A status update on this patch: (Actually a good news!)
>
> > On Jun 10, 2025, at 11:32, Qing Zhao <qing.z...@oracle.com> wrote:
> >
> >
> >
> >>>>
> >>>
> >>> It's difficult to do any meaningful pruning I think. Consider
> >>>
> >>> if (i == -1)
> >>> tem = a[i];
> >>>
> >>> when we transform this to
> >>>
> >>> if (i == -1)
> >>> tem = a[-1];
> >>>
> >>> and report the out-of-bounds access then we've lost the fact
> >>> that -1 was originally 'i' and there's an interesting condition on that
> >>> variable. So unless we preserve some kind of optimization history
> >>> on a stmt like the above we are lost. The same is true for
> >>> "altered" control edges where the most interesting are again those
> >>> that are elided by optimization (the implied true/false conditions).
> >>>
> >>> That said, I would expect the user to not use -fdiagnostics-show-context
> >>> by
> >>> default but instead when being puzzled about a diagnostic, turn it on
> >>> on-demand. So it's probably useful to report all of the chain or maybe
> >>> have -fdiagnostic-show-context=N with N limiting the depth (or a separate
> >>> knob for this).
> >>>
> >>> When there's still 'i' in a[i] but we figured out a range then we can see
> >>> to do 1. to gather conditions that we consider. The backwards threader
> >>> has "interestingness" heuristics that could eventually be repurposed.
> >>> The "good" thing is that once we decide to emit a diagnostic it's fair
> >>> to spend quite some cycles on improving it.
> >>
> >> So as a summary I'd say we'd want to have -fdiagnostic-show-context
> >> (=1 for now) and simply display the immediate control condition for
> >> the select diagnostics your patch series handled and start from there,
> >> adding test coverage where that's useful (like for my simple a[i] example
> >> above) and see in what cases that's lacking.
> >
> > Thanks for the summary, I agree.
> >
> > The following is my plan:
> >
> > 1. The first working version of -fdiagnostic-show-context=N should cover
> > all the current
> > known testing cases: all the PRs and your testing case and Kees’s new
> > testing case.
>
> I tried the following very simple heuristic to form the diagnostic path in my
> current working-in-progress patch:
>
> /* Starting from cur_bb, backtrace the CFG through the single predecessor
> to form the path. For example:
> B2
> / \
> V \
> B3 \
> / \ \
> V \---->V
> B4 B7
>
> In the above CFG, if the STMT is in B4, we backtrace to its single
> predecessor B3 first, and then back to B3's single predecessor B2,
> till there is no single predecessor anymore, or the # of backtrace
> depth exceeds the value of flag_diagnostics_show_context.
Good. Then we are sure we stop at a dominator of B4.
> For each single predecessor block, locate the conditional statement
> in the end of the block. determine whether the STMT is on the taken
> path of the condition. Add these two information to each event of
> the path. */
>
>
> The good news is: With the above simple heuristic and a simple back tracing
> of the CFG, all the
> current testing cases for the following PRs passed without any issue:
>
> PR109071
> PR88771
> PR85788
> PR108770
> PR106762
> PR115274
> PR117179
Nice. An incremental improvement would be to instead of handling
single predecessors, skip to the immediate dominator - this way
uninteresting intermediate CFG is skipped. This might be confusing
and the next "step" could be very far away, not sure how to best
handle this. But it would handle intermediate conditional code, like
if (i < 10)
{
if (dollar)
printf ("dollar");
else
printf ("euro");
printf ("%d", amout[i]);
}
where you'd not able to go up to if (i < 10). Maybe a simple heuristic
on number of line to not skip too far works, or simply ignore this
for now, or simply only use immediate dominators for the first
condition to be printed?
Thanks,
Richard.
> I will continue working on this to add more testing cases and also more
> warnings per the following discussion:
> https://gcc.gnu.org/pipermail/gcc-patches/2025-May/684126.html
> https://gcc.gnu.org/pipermail/gcc-patches/2025-June/686323.html
> https://gcc.gnu.org/pipermail/gcc-patches/2025-June/685852.html
> https://gcc.gnu.org/pipermail/gcc-patches/2025-June/685854.html
>
> Let me know if you have any comments or suggestions.
>
> Thanks a lot for all the help.
>
> Qing
> >
> > 2. Then we can improve it later if new uncovered testing case is reported.
> >
> > Is this reasonable?
> >
> > Qing
> >>
> >> Richard.
> >>
> >>>>>
> >>>>>>
>
