Bug 109071 - need more context for -Warray-bounds warnings due to code duplication from jump threading and inlining
Summary: need more context for -Warray-bounds warnings due to code duplication from ju...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 13.0
: P3 normal
Target Milestone: 16.0
Assignee: qinzhao
URL:
Keywords: diagnostic, patch
: 115704 (view as bug list)
Depends on:
Blocks: jumpthreading Warray-bounds Wformat-overflow Wstringop-overflow Wstringop-truncation Wstringop-overread
  Show dependency treegraph
 
Reported: 2023-03-08 21:04 UTC by Kees Cook
Modified: 2025-08-20 21:23 UTC (History)
7 users (show)

See Also:
Host:
Target:
Build:
Known to work: 16.0
Known to fail:
Last reconfirmed: 2023-03-09 00:00:00


Attachments
PoC for -Warray-bounds false positive (225 bytes, text/plain)
2023-03-08 21:04 UTC, Kees Cook
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kees Cook 2023-03-08 21:04:36 UTC
Created attachment 54611 [details]
PoC for -Warray-bounds false positive

The Linux kernel is seeing -Warray-bounds warnings when array indexes are being checked via inlines. This appears to be in the overly noisy/false positive territory, but I don't actually know what's going on.

The upstream report is here:
https://lore.kernel.org/lkml/20230306220947.1982272-1-trix@redhat.com/

Originally I thought this was another -fsanitizer=shift issue, but after reducing the test-case, it seems to be related to inlining or some other aspect of optimization passes.

If the "assign" function is open-coded in the caller, the warning goes away.

If the index checks are moved before the "assign" calls, the warning goes away.

If there is only 1 call to "assign", the warning goes away.

Fundamentally there should be no warning at all since the value of "index" is entirely unknown _except_ when it makes the call to "warn".

$ cat test.c
extern void warn(void);

#define MAX_ENTRIES     4

static inline void assign(int val, int *regs, int index)
{
        if (index >= MAX_ENTRIES)
                warn();
        *regs = val;
}

struct nums {
        int vals[MAX_ENTRIES];
};

void sparx5_psfp_sg_set(int *ptr, struct nums *sg, int index)
{
        int *val;

        val = &sg->vals[index];

        assign(0,    ptr, index);
        assign(*val, ptr, index);
}

$ gcc -Wall -O2  -c -o test.o test.c
test.c: In function 'sparx5_psfp_sg_set':
test.c:20:24: warning: array subscript 4 is above array bounds of 'int[4]' [-Warray-bounds=]
   20 |         val = &sg->vals[index];
      |                ~~~~~~~~^~~~~~~
test.c:13:13: note: while referencing 'vals'
   13 |         int vals[MAX_ENTRIES];
      |             ^~~~
Comment 1 Andrew Pinski 2023-03-08 21:13:55 UTC
Jump threading is happening which is causing some code to be duplicated. I am 100% sure there is a dup of this bug already filed too.
Comment 2 Richard Biener 2023-03-09 10:18:35 UTC
Yep, so we produce

<bb 2> [local count: 1073741824]:
if (index_3(D) > 3)
  goto <bb 4>; [33.00%]
else
  goto <bb 3>; [67.00%]

<bb 4> [local count: 354334800]:
warn ();
*ptr_5(D) = 0;
_17 = MEM <struct nums> [(int *)sg_2(D)].vals[index_3(D)];
warn ();

<bb 5> [local count: 1073741824]:
# _18 = PHI <_14(3), _17(4)>
*ptr_5(D) = _18;
return;

(and BB 3 with a BB4 duplicate w/o warn () calls).  If warn () were
noreturn this wouldn't happen.

And yes, we do have (plenty?) duplicates.
Comment 3 Kees Cook 2023-03-09 16:15:17 UTC
Is there a viable path to a solution here? This seems to cause enough false positives with -Warray-bounds that at least Linux can't enable the flag. I'd really like to have it enabled, though, since it finds plenty of real (and usually serious) bugs.
Comment 4 qinzhao 2023-03-10 15:51:11 UTC
(In reply to Andrew Pinski from comment #1)
> Jump threading is happening which is causing some code to be duplicated. I
> am 100% sure there is a dup of this bug already filed too.

Yes, the false positive warning is due to the code duplication by jump threading. 

***without jump threading (adding fno-thread-jumps), the IR in vrp1 is:

  <bb 2> [local count: 1073741824]:
  if (index_3(D) > 3)
    goto <bb 3>; [33.00%]
  else
    goto <bb 4>; [67.00%]

  <bb 3> [local count: 354334800]:
  warn ();

  <bb 4> [local count: 1073741824]:
  *ptr_5(D) = 0;
  _1 = MEM <struct nums> [(int *)sg_2(D)].vals[index_3(D)];
  if (index_3(D) > 3)
    goto <bb 5>; [33.00%]
  else
    goto <bb 6>; [67.00%]

  <bb 5> [local count: 354334800]:
  warn ();

***with jump threading, the "Bad" IR in vrp1 is:

  <bb 2> [local count: 1073741824]:
  if (index_3(D) > 3)
    goto <bb 4>; [33.00%]
  else
    goto <bb 3>; [67.00%]

  <bb 3> [local count: 719407024]:
  *ptr_5(D) = 0;
  _14 = MEM <struct nums> [(int *)sg_2(D)].vals[index_3(D)];
  goto <bb 5>; [100.00%]

  <bb 4> [local count: 354334800]:
  warn ();
  *ptr_5(D) = 0;
  _17 = MEM <struct nums> [(int *)sg_2(D)].vals[index_3(D)];
  warn ();

in the above "Bad" IR with jump threading, we can see the problematic part is:

  <bb 4> [local count: 354334800]:
  warn ();
  *ptr_5(D) = 0;
  _17 = MEM <struct nums> [(int *)sg_2(D)].vals[index_3(D)];
  warn ();

in which the "_17 = MEM <struct nums> [(int *)sg_2(D)].vals[index_3(D)];" is the one that was duplicated by jump threading and also is the one that caused array bound checker to report the false positive warning based on the value range propagation result in block 4: the value range for "index_3" is > 3, which is out-of-range of the array "Vals", therefore the warning. 

my though is:

1. the jump threading and code duplication are all correct optimization;
2. but the duplication of the array reference caused the false positive warning;

So, the following is my proposed solution to this problem:

1. when jump threading applies the code duplication, mark those array references that are duplicated as ARTIFICIAL (or something else..);
2. during array bound checker, check whether the array references are ARTIFICIAL, if it's true, do not emit the warning.

is the proposed solution feasible?
Comment 5 qinzhao 2024-04-22 19:12:39 UTC
adding __attribute__ ((noreturn)) to the routine "warn" can eliminate the false positive warning.
Comment 6 Kees Cook 2024-04-22 19:21:50 UTC
(In reply to qinzhao from comment #5)
> adding __attribute__ ((noreturn)) to the routine "warn" can eliminate the
> false positive warning.

But it does return... it's not an assert.
Comment 7 qinzhao 2024-04-22 20:04:45 UTC
(In reply to Kees Cook from comment #6)
> (In reply to qinzhao from comment #5)
> > adding __attribute__ ((noreturn)) to the routine "warn" can eliminate the
> > false positive warning.
> 
> But it does return... it's not an assert.

***the original code is:

if (index >= 4)
  warn ();
*regs = sg->vals[index];

if the routine "warn" does return, then
it's reasonable to convert the above original code to:

***the transformed code:

if (index >= 4)
  {
    warn ();
    *regs = sg->vals[index];   /* here, index >= 4, therefore, out-of-bound */
  }
else
  *regs = sg->vals[index];
Comment 8 Kees Cook 2024-04-22 20:15:59 UTC
The warning is about:

      val = &sg->vals[index];


  poc.c:20:20: warning: array subscript 4 is above array bounds of 'int[4]' [-Warray-bounds=]
   20 |     val = &sg->vals[index];
      |            ~~~~~~~~^~~~~~~


which happens before the warn(). And if the check is moved out of the "assign()" function, the warning goes away:


    val = &sg->vals[index];

    if (index >= MAX_ENTRIES)
        warn();

    assign(0,    ptr, index);
    assign(*val, ptr, index);

Normally -Warray-bounds doesn't warn when a value is totally unknown (i.e. "index" here can be [-INT_MAX,INT_MAX]). Why does the warning change when the MAX_ENTRIES test is moved inside assign()?
Comment 9 qinzhao 2024-04-22 21:19:03 UTC
(In reply to Kees Cook from comment #8)

> Normally -Warray-bounds doesn't warn when a value is totally unknown (i.e.
> "index" here can be [-INT_MAX,INT_MAX]). Why does the warning change when
> the MAX_ENTRIES test is moved inside assign()?

it's due to both inline transformation + thread jump optimization (and some other compiler transformation inbetween). 

***After GCC inlines both calls to "assign" into the caller "sparx5_set" and applies some other optimizations on the caller body before thread jump phase, the body of the routine "sparx5_set" is (logically):

void sparx5_set (int * ptr, struct nums * sg, int index)
{
  if (index >= 4)
    warn ();
  *ptr = 0;

  *val = sg->vals[index];
  if (index >= 4)
    warn ();
  *ptr = *val;

  return;
}

***Thread jump optimization tried to reduce the # of branches inside the routine "sparx5_set", in order to do this, sometime it needs to duplicate the code. for the above routine, after thread jump optimization, the body of the routine "sparx5_set" becomes (logically):

void sparx5_set (int * ptr, struct nums * sg, int index)
{
  if (index >= 4)
    { 
      warn ();
      *ptr = 0;               // code duplications since "warn" does return;
      *val = sg->vals[index]; // same this line. in this path, since it's under
                              // the condition "index >= 4", the compiler knows
                              // the value of "index" is larger then 4, therefore
                              // the out-of-bound warning.
      warn ();
    }
  else
    { 
      *ptr = 0;
      *val = sg->vals[index];
    }
  *ptr = *val;
  return;

}

with 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 out-of-bound warning. 

actually, I don't think that the compiler's behavior is wrong. and it's not very reasonable for the users of -Warray-bounds to assume there is zero false positive warnings. 
However, it might be reasonable to put such warnings to -Warray-bounds=2 but not in -Warray-bounds=1?
Comment 10 qinzhao 2024-05-13 14:21:30 UTC
with the following added heuristic in array-bound checker:

+  /* If the stmt is duplicated and splitted, the warning level is not 2,
+     and the current block is not dominating the exit block, not report
+     the warning.  */
+  if (is_splitted && warn_array_bounds < 2
+      && !is_dominate_exit)
+    return false;
+

such false positive warnings are moved from -Warray-bound=1 to -Warray-bound=2. [opc@qinzhao-ol8u3-x86 109071]$ /home/opc/Install/latest-d/bin/gcc -O2 -Warray-bounds=2 -c -o t.o t.c
t.c: In function ‘sparx5_set’:
t.c:12:29: warning: array subscript 4 is above array bounds of ‘int[4]’ [-Warray-bounds=]
   12 |         int *val = &sg->vals[index];
      |                     ~~~~~~~~^~~~~~~
t.c:8:18: note: while referencing ‘vals’
    8 | struct nums {int vals[4];};
      |                  ^~~~
[opc@qinzhao-ol8u3-x86 109071]$ /home/opc/Install/latest-d/bin/gcc -O2 -Warray-bounds=1 -c -o t.o t.c
[opc@qinzhao-ol8u3-x86 109071]$
Comment 11 qinzhao 2024-05-14 16:19:36 UTC
please see discussion at:
https://gcc.gnu.org/pipermail/gcc-patches/2024-May/651482.html

A summary of the discussion:

1. The current warning is correct, which catches a potential source code error that should be fixed in the source code level;
2. The best way to fix the source code level error is:
   make the routine "warn" a non-return function, and use the attribute __noreturn __ to mark it.
3. Although the warning is correct in theory, the warning message itself is confusing to the end-user since there is information that cannot be connected to the source code directly.
4. It will be a nice improvement to add more information in the warning message to report the location info on where such index value come from, for example:

warning: array index always above array bounds
events 1:

| 3 |  if (index >= 4)
        |
       (1) when index >= 4

then the end-user will know how such out-of-bound access come from.
We also need to clarify in the documentation part on how to fix such warnings in the source code level. 
5. In order to implement the above 4, we might need to record the location information to somewhere (in STMT or in Block) during the code duplication phase. more details need to be worked out on this.
Comment 12 Andrew Pinski 2024-06-29 01:58:27 UTC
*** Bug 115704 has been marked as a duplicate of this bug. ***
Comment 13 GCC Commits 2025-08-20 15:57:42 UTC
The master branch has been updated by Qing Zhao <qinzhao@gcc.gnu.org>:

https://gcc.gnu.org/g:6faa3cfe60ff9769d1bebfffdd2c7325217d7389

commit r16-3303-g6faa3cfe60ff9769d1bebfffdd2c7325217d7389
Author: Qing Zhao <qing.zhao@oracle.com>
Date:   Wed Aug 20 15:56:06 2025 +0000

    Provide new option -fdiagnostics-show-context=N for -Warray-bounds, -Wstringop-* warnings [PR109071,PR85788,PR88771,PR106762,PR108770,PR115274,PR117179]
    
    '-fdiagnostics-show-context[=DEPTH]'
    '-fno-diagnostics-show-context'
         With this option, the compiler might print the interesting control
         flow chain that guards the basic block of the statement which has
         the warning.  DEPTH is the maximum depth of the control flow chain.
         Currently, The list of the impacted warning options includes:
         '-Warray-bounds', '-Wstringop-overflow', '-Wstringop-overread',
         '-Wstringop-truncation'.  and '-Wrestrict'.  More warning options
         might be added to this list in future releases.  The forms
         '-fdiagnostics-show-context' and '-fno-diagnostics-show-context'
         are aliases for '-fdiagnostics-show-context=1' and
         '-fdiagnostics-show-context=0', respectively.
    
    For example:
    
    $ cat t.c
    extern void warn(void);
    static inline void assign(int val, int *regs, int *index)
    {
      if (*index >= 4)
        warn();
      *regs = val;
    }
    struct nums {int vals[4];};
    
    void sparx5_set (int *ptr, struct nums *sg, int index)
    {
      int *val = &sg->vals[index];
    
      assign(0,    ptr, &index);
      assign(*val, ptr, &index);
    }
    
    $ gcc -Wall -O2  -c -o t.o t.c
    t.c: In function âsparx5_setâ:
    t.c:12:23: warning: array subscript 4 is above array bounds of âint[4]â [-Warray-bounds=]
       12 |   int *val = &sg->vals[index];
          |        ~~~~~~~~^~~~~~~
    t.c:8:18: note: while referencing âvalsâ
        8 | struct nums {int vals[4];};
          |           ^~~~
    
    In the above, Although the warning is correct in theory, the warning message
    itself is confusing to the end-user since there is information that cannot
    be connected to the source code directly.
    
    It will be a nice improvement to add more information in the warning message
    to report where such index value come from.
    
    With the new option -fdiagnostics-show-context=1, the warning message for
    the above testing case is now:
    
    $ gcc -Wall -O2 -fdiagnostics-show-context=1 -c -o t.o t.c
    t.c: In function âsparx5_setâ:
    t.c:12:23: warning: array subscript 4 is above array bounds of âint[4]â [-Warray-bounds=]
       12 |   int *val = &sg->vals[index];
          |        ~~~~~~~~^~~~~~~
      âsparx5_setâ: events 1-2
        4 |   if (*index >= 4)
          |      ^
          |      |
          |      (1) when the condition is evaluated to true
    ......
       12 |   int *val = &sg->vals[index];
          |        ~~~~~~~~~~~~~~~
          |                |
          |                (2) warning happens here
    t.c:8:18: note: while referencing âvalsâ
        8 | struct nums {int vals[4];};
          |           ^~~~
    
            PR tree-optimization/109071
            PR tree-optimization/85788
            PR tree-optimization/88771
            PR tree-optimization/106762
            PR tree-optimization/108770
            PR tree-optimization/115274
            PR tree-optimization/117179
    
    gcc/ChangeLog:
    
            * Makefile.in (OBJS): Add diagnostic-context-rich-location.o.
            * common.opt (fdiagnostics-show-context): New option.
            (fdiagnostics-show-context=): New option.
            * diagnostic-context-rich-location.cc: New file.
            * diagnostic-context-rich-location.h: New file.
            * doc/invoke.texi (fdiagnostics-details): Add
            documentation for the new options.
            * gimple-array-bounds.cc (check_out_of_bounds_and_warn): Add
            one new parameter. Use rich location with details for warning_at.
            (array_bounds_checker::check_array_ref): Use rich location with
            ditails for warning_at.
            (array_bounds_checker::check_mem_ref): Add one new parameter.
            Use rich location with details for warning_at.
            (array_bounds_checker::check_addr_expr): Use rich location with
            move_history_diagnostic_path for warning_at.
            (array_bounds_checker::check_array_bounds): Call check_mem_ref with
            one more parameter.
            * gimple-array-bounds.h: Update prototype for check_mem_ref.
            * gimple-ssa-warn-access.cc (warn_string_no_nul): Use rich location
            with details for warning_at.
            (maybe_warn_nonstring_arg): Likewise.
            (maybe_warn_for_bound): Likewise.
            (warn_for_access): Likewise.
            (check_access): Likewise.
            (pass_waccess::check_strncat): Likewise.
            (pass_waccess::maybe_check_access_sizes): Likewise.
            * gimple-ssa-warn-restrict.cc (pass_wrestrict::execute): Calculate
            dominance info for diagnostics show context.
            (maybe_diag_overlap): Use rich location with details for warning_at.
            (maybe_diag_access_bounds): Use rich location with details for
            warning_at.
    
    gcc/testsuite/ChangeLog:
    
            * gcc.dg/pr109071.c: New test.
            * gcc.dg/pr109071_1.c: New test.
            * gcc.dg/pr109071_10.c: New test.
            * gcc.dg/pr109071_11.c: New test.
            * gcc.dg/pr109071_12.c: New test.
            * gcc.dg/pr109071_2.c: New test.
            * gcc.dg/pr109071_3.c: New test.
            * gcc.dg/pr109071_4.c: New test.
            * gcc.dg/pr109071_5.c: New test.
            * gcc.dg/pr109071_6.c: New test.
            * gcc.dg/pr109071_7.c: New test.
            * gcc.dg/pr109071_8.c: New test.
            * gcc.dg/pr109071_9.c: New test.
            * gcc.dg/pr117375.c: New test.
Comment 14 Sam James 2025-08-20 21:23:19 UTC
Done for 16. Thank you Qing!