[Bug c++/95693] New: Incorrect error from undefined behavior sanitizer

[Bug c++/95693] New: Incorrect error from undefined behavior sanitizer

[gcc-90 at tbilles dot hu]

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

            Bug ID: 95693
           Summary: Incorrect error from undefined behavior sanitizer
           Product: gcc
           Version: 10.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: gcc-90 at tbilles dot hu
  Target Milestone: ---

After upgrading GCC from 7.2 to 10.1 I get a runtime error when using
-fsanitize=undefined. I cannot see anything wrong with the code. It was also
suggested on the gcc-help mailing list that this is a bug in the compiler.
(https://gcc.gnu.org/pipermail/gcc-help/2020-June/139055.html)

The code in question can be found on Compiler Explorer (also pasted at the end
of this description): https://godbolt.org/z/7rAxJj

It shows that different compiler versions behave differently.

Version 10.1 prints "runtime error: reference binding to null pointer of type
'int'" although there is no null pointer in the code, the reference is bound to
a global integer.

There is a comment on line 16 that explicitly defaults the Derived constructor.
If you switch the comment with line 17, both compiler versions run fine without
producing the runtime error although the defaulted constructor should be
exactly the same the user defined one: https://godbolt.org/z/UShm-u

According to Compiler Explorer the incorrect behavior began in GCC 8.x series
and is still present in trunk.

---
int global = 9;

class Payload {
public:
    Payload() : data(global) {}
private:
    int& data;
};

struct Base {
    Payload payload;
};

class Derived : public Base {
public:
    //Derived() = default;
    Derived() : Base() {}

    Payload p;
};

int main()
{
    Derived t;
}

[Bug sanitizer/95693] [8/9/10/11 Regression] Incorrect error from undefined behavior sanitizer

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dodji at gcc dot gnu.org,
                   |                            |dvyukov at gcc dot gnu.org,
                   |                            |jakub at gcc dot gnu.org,
                   |                            |kcc at gcc dot gnu.org,
                   |                            |marxin at gcc dot gnu.org
      Known to work|                            |7.2.0
           Keywords|                            |wrong-code
   Target Milestone|---                         |8.5
          Component|c++                         |sanitizer
            Summary|Incorrect error from        |[8/9/10/11 Regression]
                   |undefined behavior          |Incorrect error from
                   |sanitizer                   |undefined behavior
                   |                            |sanitizer

[Bug sanitizer/95693] [8/9/10/11 Regression] Incorrect error from undefined behavior sanitizer

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
                 CC|                            |jason at gcc dot gnu.org,
                   |                            |mpolacek at gcc dot gnu.org,
                   |                            |nathan at gcc dot gnu.org
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-06-16

--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
Thank you for the report!
The error is newly printed with r8-7149-g2e1a7ecb2d8f1ee3.

Backtrace is here:
g++ pr95693.C -fsanitize=undefined -g && ./a.out 
pr95693.C:17:22: runtime error: reference binding to null pointer of type 'int'
    #0 0x4011fb in Derived::Derived()
/home/marxin/Programming/testcases/pr95693.C:17
    #1 0x401135 in main /home/marxin/Programming/testcases/pr95693.C:24
    #2 0x7ffff7147cc9 in __libc_start_main ../csu/libc-start.c:308
    #3 0x401079 in _start (/home/marxin/Programming/testcases/a.out+0x401079)

I'm not a C++ expert, but it seems to me an invalid code.

[Bug sanitizer/95693] [8/9/10/11 Regression] Incorrect error from undefined behavior sanitizer

[jakub at gcc dot gnu.org]

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The Derived ctor seems to first perform zero initialization of the base for
some reason and only then calls the Base constructor.  Dunno if that is
required or not, but since Alex' change actually also zeroes the reference. 
And this state before calling the Base is caught by the sanitizer.  So, to me
it looks like a C++ FE problem that it zero initializes it first, or if it
needs to, then it needs to either avoid what Alex' patch is doing (do that only
during error-recovery), or arrange somehow that it will not be sanitized.

[Bug sanitizer/95693] [8/9/10/11 Regression] Incorrect error from undefined behavior sanitizer

[redi at gcc dot gnu.org]

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

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Martin Liška from comment #1)
> I'm not a C++ expert, but it seems to me an invalid code.

I don't see anything wrong with the code.

[Bug sanitizer/95693] [8/9/10/11 Regression] Incorrect error from undefined behavior sanitizer

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

[Bug sanitizer/95693] [8/9/10/11 Regression] Incorrect error from undefined behavior sanitizer

[nathan at gcc dot gnu.org]

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

--- Comment #4 from Nathan Sidwell <nathan at gcc dot gnu.org> ---
there;s no language reason to zero init the base.  It looks to me as if Alex's
change is compensating for a problem in the pic code he cites.  Is that code
trying to make a PIC initializer for something that is not initialized (in
erroneous code)?

[Bug sanitizer/95693] [8/9/10/11 Regression] Incorrect error from undefined behavior sanitizer

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 50019
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50019&action=edit
gcc11-pr95693.patch

This untested patch reverts Alex' build_zero_init* change and instead does it
in the caller where we know it is only for error recovery (we've emitted an
error earlier about it).

[Bug sanitizer/95693] [8/9/10/11 Regression] Incorrect error from undefined behavior sanitizer

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

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

commit r11-6865-ga9ed18295bfc6d69d40af197e059e16622cd94c6
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Jan 22 19:03:23 2021 +0100

    c++: Fix up ubsan false positives on references [PR95693]

    Alex' 2 years old change to build_zero_init_1 to return NULL pointer with
    reference type for references breaks the sanitizers, the assignment of NULL
    to a reference typed member is then instrumented before it is overwritten
    with a non-NULL address later on.
    That change has been done to fix error recovery ICE during
    process_init_constructor_record, where we:
              if (TYPE_REF_P (fldtype))
                {
                  if (complain & tf_error)
                    error ("member %qD is uninitialized reference", field);
                  else
                    return PICFLAG_ERRONEOUS;
                }
    a few lines earlier, but then continue and ICE when build_zero_init returns
    NULL.

    The following patch reverts the build_zero_init_1 change and instead
creates
    the NULL with reference type constants during the error recovery.

    The pr84593.C testcase Alex' change was fixing still works as before.

    2021-01-22  Jakub Jelinek  <jakub@redhat.com>

            PR sanitizer/95693
            * init.c (build_zero_init_1): Revert the 2018-03-06 change to
            return build_zero_cst for reference types.
            * typeck2.c (process_init_constructor_record): Instead call
            build_zero_cst here during error recovery instead of
build_zero_init.

            * g++.dg/ubsan/pr95693.C: New test.

[Bug sanitizer/95693] [8/9/10 Regression] Incorrect error from undefined behavior sanitizer

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[8/9/10/11 Regression]      |[8/9/10 Regression]
                   |Incorrect error from        |Incorrect error from
                   |undefined behavior          |undefined behavior
                   |sanitizer                   |sanitizer

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed on the trunk so far.

[Bug sanitizer/95693] [8/9/10 Regression] Incorrect error from undefined behavior sanitizer

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

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

commit r10-9318-ge5750f847158e7f9bdab770fd9c5fff58c5074d3
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Jan 22 19:03:23 2021 +0100

    c++: Fix up ubsan false positives on references [PR95693]

    Alex' 2 years old change to build_zero_init_1 to return NULL pointer with
    reference type for references breaks the sanitizers, the assignment of NULL
    to a reference typed member is then instrumented before it is overwritten
    with a non-NULL address later on.
    That change has been done to fix error recovery ICE during
    process_init_constructor_record, where we:
              if (TYPE_REF_P (fldtype))
                {
                  if (complain & tf_error)
                    error ("member %qD is uninitialized reference", field);
                  else
                    return PICFLAG_ERRONEOUS;
                }
    a few lines earlier, but then continue and ICE when build_zero_init returns
    NULL.

    The following patch reverts the build_zero_init_1 change and instead
creates
    the NULL with reference type constants during the error recovery.

    The pr84593.C testcase Alex' change was fixing still works as before.

    2021-01-22  Jakub Jelinek  <jakub@redhat.com>

            PR sanitizer/95693
            * init.c (build_zero_init_1): Revert the 2018-03-06 change to
            return build_zero_cst for reference types.
            * typeck2.c (process_init_constructor_record): Instead call
            build_zero_cst here during error recovery instead of
build_zero_init.

            * g++.dg/ubsan/pr95693.C: New test.

[Bug sanitizer/95693] [8/9 Regression] Incorrect error from undefined behavior sanitizer

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[8/9/10 Regression]         |[8/9 Regression] Incorrect
                   |Incorrect error from        |error from undefined
                   |undefined behavior          |behavior sanitizer
                   |sanitizer                   |

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for 10.3+ too.

[Bug sanitizer/95693] [8/9 Regression] Incorrect error from undefined behavior sanitizer

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:4ccdb3fdbc14102c91b6148bcbe09d0763726ae0

commit r9-9408-g4ccdb3fdbc14102c91b6148bcbe09d0763726ae0
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Jan 22 19:03:23 2021 +0100

    c++: Fix up ubsan false positives on references [PR95693]

    Alex' 2 years old change to build_zero_init_1 to return NULL pointer with
    reference type for references breaks the sanitizers, the assignment of NULL
    to a reference typed member is then instrumented before it is overwritten
    with a non-NULL address later on.
    That change has been done to fix error recovery ICE during
    process_init_constructor_record, where we:
              if (TYPE_REF_P (fldtype))
                {
                  if (complain & tf_error)
                    error ("member %qD is uninitialized reference", field);
                  else
                    return PICFLAG_ERRONEOUS;
                }
    a few lines earlier, but then continue and ICE when build_zero_init returns
    NULL.

    The following patch reverts the build_zero_init_1 change and instead
creates
    the NULL with reference type constants during the error recovery.

    The pr84593.C testcase Alex' change was fixing still works as before.

    2021-01-22  Jakub Jelinek  <jakub@redhat.com>

            PR sanitizer/95693
            * init.c (build_zero_init_1): Revert the 2018-03-06 change to
            return build_zero_cst for reference types.
            * typeck2.c (process_init_constructor_record): Instead call
            build_zero_cst here during error recovery instead of
build_zero_init.

            * g++.dg/ubsan/pr95693.C: New test.

    (cherry picked from commit e5750f847158e7f9bdab770fd9c5fff58c5074d3)

[Bug sanitizer/95693] [8/9 Regression] Incorrect error from undefined behavior sanitizer

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-8 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

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

commit r8-10875-ga3458b238e1297ad7ab6f92ab1a00837f282691a
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Jan 22 19:03:23 2021 +0100

    c++: Fix up ubsan false positives on references [PR95693]

    Alex' 2 years old change to build_zero_init_1 to return NULL pointer with
    reference type for references breaks the sanitizers, the assignment of NULL
    to a reference typed member is then instrumented before it is overwritten
    with a non-NULL address later on.
    That change has been done to fix error recovery ICE during
    process_init_constructor_record, where we:
              if (TYPE_REF_P (fldtype))
                {
                  if (complain & tf_error)
                    error ("member %qD is uninitialized reference", field);
                  else
                    return PICFLAG_ERRONEOUS;
                }
    a few lines earlier, but then continue and ICE when build_zero_init returns
    NULL.

    The following patch reverts the build_zero_init_1 change and instead
creates
    the NULL with reference type constants during the error recovery.

    The pr84593.C testcase Alex' change was fixing still works as before.

    2021-01-22  Jakub Jelinek  <jakub@redhat.com>

            PR sanitizer/95693
            * init.c (build_zero_init_1): Revert the 2018-03-06 change to
            return build_zero_cst for reference types.
            * typeck2.c (process_init_constructor_record): Instead call
            build_zero_cst here during error recovery instead of
build_zero_init.

            * g++.dg/ubsan/pr95693.C: New test.

    (cherry picked from commit e5750f847158e7f9bdab770fd9c5fff58c5074d3)

[Bug sanitizer/95693] [8/9 Regression] Incorrect error from undefined behavior sanitizer

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.

[Bug sanitizer/95693] [8/9 Regression] Incorrect error from undefined behavior sanitizer

[gcc-90 at tbilles dot hu]

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

--- Comment #13 from Tibor Billes <gcc-90 at tbilles dot hu> ---
Thank you all for fixing it!