[Bug c/116458] New: New valgrind error in search_line_ssse3

[Bug c/116458] New: New valgrind error in search_line_ssse3

[dcb314 at hotmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116458

            Bug ID: 116458
           Summary: New valgrind error in search_line_ssse3
           Product: gcc
           Version: 15.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dcb314 at hotmail dot com
  Target Milestone: ---

From today's valgrind build of gcc trunk:

/usr/bin/valgrind -q build/genmatch --generic \
    --header=tmp-generic-match-auto.h --include=generic-match-auto.h \
    ../../trunk/gcc/match.pd  tmp-generic-match-1.cc  tmp-generic-match-2.cc 
tmp-generic-match-3.cc  tmp-generic-match-4.cc  tmp-generic-match-5.cc 
tmp-generic-match-6.cc  tmp-generic-match-7.cc  tmp-generic-match-8.cc 
tmp-generic-match-9.cc  tmp-generic-match-10.cc
==238807== Conditional jump or move depends on uninitialised value(s)
==238807==    at 0x42D5B4: search_line_ssse3(unsigned char const*, unsigned
char const*) (lex.cc:378)
==238807==    by 0x42E222: _cpp_clean_line (lex.cc:845)
==238807==    by 0x42E6D2: bool get_fresh_line_impl<false>(cpp_reader*)
(lex.cc:3635)

File lex.cc, line 378 is

  while (!found);

Configure script is

../trunk/configure  \
        --disable-multilib \
        --disable-bootstrap \
        --enable-checking=valgrind \
        --enable-languages=c,c++

git log libcpp/lex.cc says:

commit 20a5b4824993ae1c99f3b965c5e07bbd2c64b2ce
Author: Alexander Monakov <amonakov@ispras.ru>
Date:   Tue Aug 6 09:47:23 2024 +0300

    libcpp: replace SSE4.2 helper with an SSSE3 one

[Bug c/116458] New valgrind error in search_line_ssse3

[dcb314 at hotmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116458

David Binderman <dcb314 at hotmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amonakov at gcc dot gnu.org

--- Comment #1 from David Binderman <dcb314 at hotmail dot com> ---
Adding author of most recent commit for their opinion.

[Bug c/116458] New valgrind error in search_line_ssse3

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116458

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
It's likely the tail padding we possibly inspect, with now unrolling the loop
twice to improve the number of badly predictable branches we can now end up
with inspecting a completely uninitialized qword.  This possibly makes
valgrind fail to realize that uninit data doesn't actually influence the
jump so IMO it's a valgrind issue.

[Bug c/116458] [15 regression] New valgrind error in search_line_ssse3

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116458

--- Comment #3 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
David, thanks for Cc'ing me and for running Valgrind builds!

Richi, I'll check in more detail later today, I think we should unbreak
Valgrind builds ASAP by initializing padding under #ifdef
ENABLE_VALGRIND_WORKAROUNDS.

Your explanation makes sense to me, but it's pointing to a pedantically real
issue (the code relies on never getting "wobbly values" from uninit reads, or
else it needs asm propagation barriers or atomic-relaxed loads to ensure the
values are really loaded just once, but neither would be visible to Valgrind in
the end).

"Wobbly values" aside, judging from how this never arised with other vectorized
helpers, Valgrind treats some instructions optimistically? Probably pmovmskb
from a partially-uninit vector yields a fully-initialized mask? If so, making
the new helper work as well without workarounds should be doable (I'll see if I
can come up with a testcase for Valgrind).

[Bug c/116458] [15 regression] New valgrind error in search_line_ssse3

[dcb314 at hotmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116458

--- Comment #4 from David Binderman <dcb314 at hotmail dot com> ---
(In reply to Alexander Monakov from comment #3)
> David, thanks for Cc'ing me and for running Valgrind builds!

You are welcome. Its a normal weekly part of gcc testing for me.

> "Wobbly values" aside, judging from how this never arised with other
> vectorized helpers, Valgrind treats some instructions optimistically?

I saw the problem first in a git development version of valgrind,
but was able to duplicate it with the released version 3.23.0.

I see no problems in this area with asan and ubsan.

[Bug preprocessor/116458] [15 regression] New valgrind error in search_line_ssse3

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116458

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |15.0
               Host|                            |X86_64
          Component|c                           |preprocessor

[Bug preprocessor/116458] [15 regression] New valgrind error in search_line_ssse3

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116458

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2024-08-22
           Assignee|unassigned at gcc dot gnu.org      |amonakov at gcc dot gnu.org

--- Comment #5 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Turns out we already initialize padding, just in a different file, and I
completely overlooked that. That's why the SSE4.2 helper did not cause Valgrind
errors.

https://inbox.sourceware.org/gcc-patches/20240822182508.621-1-amonakov@ispras.ru/

[Bug preprocessor/116458] [15 regression] New valgrind error in search_line_ssse3

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116458

--- Comment #6 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
As for Valgrind false positive, it handles this SSSE3 code really well and
misses the key point by a very narrow margin. We have

  found = m1 + (m2 << 16);

where both m1 and m2 hold 16-bit masks from pmovmskb, so 'found' is just the
32-bit concatenation of those masks.

Suppose m1 has uninit bits. In that case, its highest known bit is guaranteed
to be non-zero (because our character stream always ends with a newline), in
'm2 << 16' the lower 16 bits are known, and therefore 'found' has a known
non-zero bit, making the branch on found != 0 well-defined.

If we instead combine the halves with an OR instead of PLUS:

  found = m1 | (m2 << 16);

Valgrind properly computes known bits in 'found' and does not complain.

I can see the difficulty in computing known bits after addition in the general
case though.

[Bug preprocessor/116458] [15 regression] New valgrind error in search_line_ssse3

[mark at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116458

--- Comment #7 from Mark Wielaard <mark at gcc dot gnu.org> ---
You could try --expensive-definedness-checks=yes

       --expensive-definedness-checks=<no|auto|yes> [default: auto]
           Controls whether Memcheck should employ more precise but also more
           expensive (time consuming) instrumentation when checking the
           definedness of certain values. In particular, this affects the
           instrumentation of integer adds, subtracts and equality
           comparisons.

           Selecting --expensive-definedness-checks=yes causes Memcheck to use
           the most accurate analysis possible. This minimises false error
           rates but can cause up to 30% performance degradation.

           Selecting --expensive-definedness-checks=no causes Memcheck to use
           the cheapest instrumentation possible. This maximises performance
           but will normally give an unusably high false error rate.

           The default setting, --expensive-definedness-checks=auto, is
           strongly recommended. This causes Memcheck to use the minimum of
           expensive instrumentation needed to achieve the same false error
           rate as --expensive-definedness-checks=yes. It also enables an
           instrumentation-time analysis pass which aims to further reduce the
           costs of accurate instrumentation. Overall, the performance loss is
           generally around 5% relative to --expensive-definedness-checks=no,
           although this is strongly workload dependent. Note that the exact
           instrumentation settings in this mode are architecture dependent.

[Bug preprocessor/116458] [15 regression] New valgrind error in search_line_ssse3

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116458

--- Comment #8 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Thanks for the reference, but it doesn't help. Something more subtle is going
on, because placing the shift-add combo in a separate function makes Valgrind
properly compute known bits even without the magic flag. Here's a testcase:

__attribute__((noipa))
static int f(short *s)
{
        return s[0] + (s[1] << 16);
}

int main()
{
        void *p = __builtin_malloc(64);
        *(char *)p = 1;
        asm("" ::: "memory");
        if (!f(p))
                __builtin_abort();
}

Compile with gcc -O2 and run under valgrind. Then comment out the attribute,
recompile (gcc will inline the function) and run again — valgrind will complain

==2617== Conditional jump or move depends on uninitialised value(s)
==2617==    at 0x10908D: main (t.c:12)

[Bug preprocessor/116458] [15 regression] New valgrind error in search_line_ssse3

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116458

--- Comment #9 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Okay, if you take the addition and the branch from the inlined variant:

addl %eax, %edx
je .L3

and add a 'test' instruction:

addl %eax, %edx
test %edx, %edx
je .L3

then Valgrind doesn't complain. So it looks like propagation of known bits is
actually powerful enough, but there's a strange weakness in handling of ZF when
it is used directly from addition.

[Bug preprocessor/116458] [15 regression] New valgrind error in search_line_ssse3

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116458

--- Comment #10 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Alexander Monakov <amonakov@gcc.gnu.org>:

https://gcc.gnu.org/g:b2c1d7c4573d3b938f44b3bda202adeb292b1cbc

commit r15-3121-gb2c1d7c4573d3b938f44b3bda202adeb292b1cbc
Author: Alexander Monakov <amonakov@ispras.ru>
Date:   Thu Aug 22 21:09:47 2024 +0300

    libcpp: bump padding size in _cpp_convert_input [PR116458]

    The recently introduced search_line_fast_ssse3 raised padding
    requirement from 16 to 64, which was adjusted in read_file_guts,
    but the corresponding ' + 16' in _cpp_convert_input was overlooked.

    libcpp/ChangeLog:

            PR preprocessor/116458
            * charset.cc (_cpp_convert_input): Bump padding to 64 if
            HAVE_SSSE3.

[Bug preprocessor/116458] [15 regression] New valgrind error in search_line_ssse3

[dcb314 at hotmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116458

--- Comment #11 from David Binderman <dcb314 at hotmail dot com> ---
I confirm that the fix works for me. 

On a more subtle note, if two functions are strongly related,
would it be wise to have them in the same file with some comments
and maybe even some asserts to ensure compatibility ?

It might prevent similar errors of this type in future.

[Bug preprocessor/116458] [15 regression] New valgrind error in search_line_ssse3

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116458

--- Comment #12 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Thanks. It's probably nicer to deduplicate computation of required padding to a
common header (libcpp/internal.h), I'll send a patch to that effect.

[Bug preprocessor/116458] [15 regression] New valgrind error in search_line_ssse3

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116458

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #13 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
(In reply to Alexander Monakov from comment #12)
> Thanks. It's probably nicer to deduplicate computation of required padding
> to a common header (libcpp/internal.h), I'll send a patch to that effect.

That was
https://inbox.sourceware.org/gcc-patches/E07C440F-C8EC-4C02-AA9E-E93F4EEE424D@gmail.com/T/

My bugreport for Valgrind false positive is at
https://bugs.kde.org/show_bug.cgi?id=492210

All done, closing.