Bug 101631 - gcc allows for the changing of an union active member to be changed via a reference
Summary: gcc allows for the changing of an union active member to be changed via a ref...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 12.0
: P3 normal
Target Milestone: 14.0
Assignee: Not yet assigned to anyone
URL:
Keywords: accepts-invalid
: 98637 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-07-26 18:46 UTC by fsb4000
Modified: 2024-11-15 23:40 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2021-07-26 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description fsb4000 2021-07-26 18:46:39 UTC
Hi.

The bug has come up during the implementation of SSO for constexpr std::string which can be found here: https://github.com/microsoft/STL/pull/1735#discussion_r674285711

The issue stems from an obscure rule(https://eel.is/c++draft/class.union.general#6) that allows switching the active member of a union by assigning to an element of the array member. (Obligatory godbolt: https://godbolt.org/z/6qG7v9eYx)

While the usage on line 12 is indeed correct, for std::string we need to go through char_traits which is imitated by perform_assignment. However, the lifetime of the subobject buf[5] only starts right before the assignment inside of perform_assignment, so on line 25 we form an object reference to an object outside of its lifetime which is ill formed.

gcc should emit an appropriate diagnostic.

Found by: Michael Schellenberger Costa

I hope it helps.
Comment 1 Jonathan Wakely 2021-07-26 20:45:10 UTC
(In reply to fsb4000 from comment #0)
> (Obligatory godbolt: https://godbolt.org/z/6qG7v9eYx)

Godbolt links are not obligatory, but a testcase is. See https://gcc.gnu.org/bugs
Comment 2 fsb4000 2021-07-27 02:32:26 UTC
Sure.

$ g++ -c -std=c++20 -save-temps main.cpp

$ g++ -v
Using built-in specs.
COLLECT_GCC=C:\tools\msys64\mingw64\bin\g++.exe
COLLECT_LTO_WRAPPER=C:/tools/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.3.0/lto-wrapper.exe
Target: x86_64-w64-mingw32
Configured with: ../gcc-10.3.0/configure --prefix=/mingw64 --with-local-prefix=/mingw64/local --build=x86_64-w64-mingw32 --host=x86_64-w64-mingw32 --target=x86_64-w64-mingw32 --with-native-system-header-dir=/mingw64/x86_64-w64-mingw32/include --libexecdir=/mingw64/lib --enable-bootstrap --enable-checking=release --with-arch=x86-64 --with-tune=generic --enable-languages=c,lto,c++,fortran,ada,objc,obj-c++,jit --enable-shared --enable-static --enable-libatomic --enable-threads=posix --enable-graphite --enable-fully-dynamic-string --enable-libstdcxx-filesystem-ts=yes --enable-libstdcxx-time=yes --disable-libstdcxx-pch --disable-libstdcxx-debug --enable-lto --enable-libgomp --disable-multilib --disable-rpath --disable-win32-registry --disable-nls --disable-werror --disable-symvers --with-libiconv --with-system-zlib --with-gmp=/mingw64 --with-mpfr=/mingw64 --with-mpc=/mingw64 --with-isl=/mingw64 --with-pkgversion='Rev5, Built by MSYS2 project' --with-bugurl=https://github.com/msys2/MINGW-packages/issues --with-gnu-as --with-gnu-ld --with-boot-ldflags='-pipe -Wl,--dynamicbase,--high-entropy-va,--nxcompat,--default-image-base-high -Wl,--disable-dynamicbase -static-libstdc++ -static-libgcc' 'LDFLAGS_FOR_TARGET=-pipe -Wl,--dynamicbase,--high-entropy-va,--nxcompat,--default-image-base-high' --enable-linker-plugin-flags='LDFLAGS=-static-libstdc++\ -static-libgcc\ -pipe\ -Wl,--dynamicbase,--high-entropy-va,--nxcompat,--default-image-base-high\ -Wl,--stack,12582912'
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 10.3.0 (Rev5, Built by MSYS2 project)

$ cat main.ii
# 1 "main.cpp"
# 1 "<built-in>"
# 1 "<command-line>"
# 1 "main.cpp"
struct sso {
    union {
        int buf[10];
        int* alloc;
    };
};

constexpr bool test_switch() noexcept {
    sso val;
    val.alloc = nullptr;
    val.buf[5] = 42;
    return true;
}
static_assert(test_switch());

constexpr void perform_assignment(int& left, int right) noexcept {
    left = right;
}

constexpr bool test_switch_with_indirection() noexcept {
    sso val;
    val.alloc = nullptr;
    perform_assignment(val.buf[5], 42);
    return true;
}
static_assert(test_switch_with_indirection());


Expected: 

static_assert(test_switch_with_indirection());

should fail.

Sceenshot(just in case ): https://imgur.com/a/DXYYq1y

My OS: Windows 10 Pro 64 bit, 21H1


I'm confused with "What we do not want: Bugs in releases or snapshots of GCC not issued by the GNU Project. Report them to whoever provided you with the release".
What should I do? I can check at godbolt.org or locally using gcc from msys2. But it's stupid to report gcc bugs to people working on godbolt.org or msys2. They are not gcc developers. 

BTW the bug report in the first comment is almost identical to bug report we sent to Visual C++ developers: https://developercommunity.visualstudio.com/t/cl-permits-object-reference-to-object-outside-of-i/1487178 (just cl instead of gcc)
Comment 3 Jonathan Wakely 2021-07-27 14:57:49 UTC
(In reply to fsb4000 from comment #2)
> I'm confused with "What we do not want: Bugs in releases or snapshots of GCC
> not issued by the GNU Project. Report them to whoever provided you with the
> release".

10.3.0 is an official release. 10.3.1 or some heavily patched GCC would not be.
Comment 4 GCC Commits 2023-10-20 03:26:10 UTC
The trunk branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:1d260ab0e39ea63644e3af3ab2e0db946026b5a6

commit r14-4771-g1d260ab0e39ea63644e3af3ab2e0db946026b5a6
Author: Nathaniel Shead <nathanieloshead@gmail.com>
Date:   Thu Oct 12 19:53:55 2023 +1100

    c++: indirect change of active union member in constexpr [PR101631,PR102286]
    
    This patch adds checks for attempting to change the active member of a
    union by methods other than a member access expression.
    
    To be able to properly distinguish `*(&u.a) = ` from `u.a = `, this
    patch redoes the solution for c++/59950 to avoid extranneous *&; it
    seems that the only case that needed the workaround was when copying
    empty classes.
    
    This patch also ensures that constructors for a union field mark that
    field as the active member before entering the call itself; this ensures
    that modifications of the field within the constructor's body don't
    cause false positives (as these will not appear to be member access
    expressions). This means that we no longer need to start the lifetime of
    empty union members after the constructor body completes.
    
    As a drive-by fix, this patch also ensures that value-initialised unions
    are considered to have activated their initial member for the purpose of
    checking stores and accesses, which catches some additional mistakes
    pre-C++20.
    
            PR c++/101631
            PR c++/102286
    
    gcc/cp/ChangeLog:
    
            * call.cc (build_over_call): Fold more indirect refs for trivial
            assignment op.
            * class.cc (type_has_non_deleted_trivial_default_ctor): Create.
            * constexpr.cc (cxx_eval_call_expression): Start lifetime of
            union member before entering constructor.
            (cxx_eval_component_reference): Check against first member of
            value-initialised union.
            (cxx_eval_store_expression): Activate member for
            value-initialised union. Check for accessing inactive union
            member indirectly.
            * cp-tree.h (type_has_non_deleted_trivial_default_ctor):
            Forward declare.
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/cpp1y/constexpr-89336-3.C: Fix union initialisation.
            * g++.dg/cpp1y/constexpr-union6.C: New test.
            * g++.dg/cpp1y/constexpr-union7.C: New test.
            * g++.dg/cpp2a/constexpr-union2.C: New test.
            * g++.dg/cpp2a/constexpr-union3.C: New test.
            * g++.dg/cpp2a/constexpr-union4.C: New test.
            * g++.dg/cpp2a/constexpr-union5.C: New test.
            * g++.dg/cpp2a/constexpr-union6.C: New test.
    
    Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
    Reviewed-by: Jason Merrill <jason@redhat.com>
Comment 5 Sam James 2023-10-25 06:04:11 UTC
Fixed for 14 then by the drive-by bit or is there something left?
Comment 6 Nathaniel Shead 2023-10-26 00:19:28 UTC
(In reply to Sam James from comment #5)
> Fixed for 14 then by the drive-by bit or is there something left?
It should be fixed now. The example in comment #2 is g++.dg/cpp2a/constexpr-union2.C.
Comment 7 Patrick Palka 2023-10-26 14:58:04 UTC
Marking this fixed for GCC 14 then, thanks!
Comment 8 Patrick Palka 2023-12-12 18:45:34 UTC
*** Bug 98637 has been marked as a duplicate of this bug. ***