Confirmed.
      else if (!fndecl_has_gimple_body_p (callee_fndecl)
               && (!(callee_fndecl_flags & (ECF_CONST | ECF_PURE)))
               && !fndecl_built_in_p (callee_fndecl))
        unknown_side_effects = true;

The last part is part of the problem I think. At least here.
Yes maybe we should have another builtin which returns a "_Complex unsigned long long" here which is folded into for __builtin_ia32_rdrand*_step to remove the need to the address too.

I am going to declare this one as a target issue but there might be other builtins which are harder to do the "_Complex" trick.

(In reply to Andrew Pinski from comment #1)
> Confirmed.
>       else if (!fndecl_has_gimple_body_p (callee_fndecl)
>                && (!(callee_fndecl_flags & (ECF_CONST | ECF_PURE)))
>                && !fndecl_built_in_p (callee_fndecl))
>         unknown_side_effects = true;
> 
> The last part is part of the problem I think. At least here.

I think the problem is here, in the analyzer: I think the analyzer is here making the assumption that builtins that haven't been explicitly handled don't have side effects (such as writing through the input pointers), which is clearly wrong for this builtin.


> Yes maybe we should have another builtin which returns a "_Complex unsigned
> long long" here which is folded into for __builtin_ia32_rdrand*_step to
> remove the need to the address too.
> 
> I am going to declare this one as a target issue but there might be other
> builtins which are harder to do the "_Complex" trick.

Andrew: I see you've marked this as a target missed-optimization bug, but arguably there's still an analyzer bug here.  Should we reassign this back to the analyzer, or perhaps make a clone of the bug so that we can cover the two aspects of this separately?

I also think it's an analyzer problem, the && !fndecl_built_in_p (callee_fndecl) is broken IMHO.

Created attachment 54565 [details]
Patch that reworks builtin handling

I've been testing this patch, but it might be too invasive at this point in GCC 13; attaching here to back it up while I try a less invasive approach.

The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:24ebc5404b88b765221b551dc5288f6d64ba3dc7

commit r13-6398-g24ebc5404b88b765221b551dc5288f6d64ba3dc7
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Wed Mar 1 17:24:32 2023 -0500

    analyzer: fixes to side-effects for built-in functions [PR107565]
    
    Previously, if the analyzer saw a call to a non-pure and non-const
    built-in function that it didn't have explicit knowledge of the behavior
    of, it would fall back to assuming that the builtin could have arbitrary
    behavior, similar to a function defined outside of the current TU.
    
    However, this only worked for BUILTIN_NORMAL functions that matched
    gimple_builtin_call_types_compatible_p; for BUILT_IN_FRONTEND and
    BUILT_IN_MD, and for mismatched types the analyzer would erroneously
    assume that the builtin had no side-effects, leading e.g. to
    PR analyzer/107565, where the analyzer falsely reported that x
    was still uninitialized after this target-specific builtin:
    
      _1 = __builtin_ia32_rdrand64_step (&x);
    
    This patch generalizes the handling to cover all classes of builtin,
    fixing the above false positive.
    
    Unfortunately this patch regresses gcc.dg/analyzer/pr99716-1.c due to
    the:
      fprintf (fp, "hello");
    being optimized to:
       __builtin_fwrite ("hello", 1, (ssizetype)5, fp_6);
    and the latter has gimple_builtin_call_types_compatible_p return false,
    whereas the original call had it return true.  I'm assuming that this is
    an optimization bug, and have filed it as PR middle-end/108988.  The
    effect on the analyzer is that it fails to recognize the call to
    __builtin_fwrite and instead assumes arbitraty side-effects (including
    that it could call fclose on fp, hence the report about the leak goes
    away).
    
    I tried various more involved fixes with new heuristics for handling
    built-ins that aren't explicitly covered by the analyzer, but those
    fixes tended to introduce many more regressions, so I'm going with this
    simpler fix.
    
    gcc/analyzer/ChangeLog:
            PR analyzer/107565
            * region-model.cc (region_model::on_call_pre): Flatten logic by
            returning early.  Consolidate logic for detecting const and pure
            functions.  When considering whether an unhandled built-in
            function has side-effects, consider all kinds of builtin, rather
            than just BUILT_IN_NORMAL, and don't require
            gimple_builtin_call_types_compatible_p.
    
    gcc/testsuite/ChangeLog:
            PR analyzer/107565
            * gcc.dg/analyzer/builtins-pr107565.c: New test.
            * gcc.dg/analyzer/pr99716-1.c (test_2): Mark the leak as xfailing.
    
    Signed-off-by: David Malcolm <dmalcolm@redhat.com>

Should be fixed on trunk for GCC 13 by the above patch.

Keeping this report open for now to track backporting the fix to GCC 12.

The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:56572a08ec4a0fc1a7802d3737cd7f7cc9089c4b

commit r13-6466-g56572a08ec4a0fc1a7802d3737cd7f7cc9089c4b
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Fri Mar 3 17:59:21 2023 -0500

    analyzer: provide placeholder implementation of sprintf
    
    Previously, the analyzer lacked a known_function implementation of
    sprintf, and thus would handle calls to sprintf with the "anything could
    happen" fallback.
    
    Whilst working on PR analyzer/107565 I noticed that this was preventing
    a lot of genuine memory leaks from being reported for Doom; fixing
    thusly.
    
    Integration testing of the effect of the patch shows a big increase in
    true positives due to the case mentioned in Doom, and one new false
    positive (in pcre2), which I'm tracking as PR analyzer/109014.
    
    Comparison:
      GOOD:  67 -> 123 (+56); 10.91% -> 18.33%
       BAD: 547 -> 548 (+1)
    
    where the affected warnings/projects are:
    
      -Wanalyzer-malloc-leak:
        GOOD:  0 -> 56 (+56);  0.00% -> 41.48%
         BAD: 79
          True positives: 0 -> 56 (+56)
            (all in Doom)
    
      -Wanalyzer-use-of-uninitialized-value:
        GOOD: 0;  0.00%
         BAD: 80 -> 81 (+1)
          False positives:
            pcre2-10.42: 0 -> 1 (+1)
    
    gcc/analyzer/ChangeLog:
            * kf.cc (class kf_sprintf): New.
            (register_known_functions): Register it.
    
    gcc/testsuite/ChangeLog:
            * gcc.dg/analyzer/doom-d_main-IdentifyVersion.c: New test.
            * gcc.dg/analyzer/sprintf-1.c: New test.
            * gcc.dg/analyzer/sprintf-concat.c: New test.
    
    Signed-off-by: David Malcolm <dmalcolm@redhat.com>

GCC 12.3 is being released, retargeting bugs to GCC 12.4.

GCC 12.4 is being released, retargeting bugs to GCC 12.5.