Bug 88115 - Incorrect result from alignof in templates, if also using __alignof__.
Summary: Incorrect result from alignof in templates, if also using __alignof__.
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 8.2.1
: P3 normal
Target Milestone: 8.5
Assignee: Patrick Palka
URL:
Keywords: ABI, wrong-code
Depends on:
Blocks:
 
Reported: 2018-11-20 17:31 UTC by James Y Knight
Modified: 2021-04-21 12:10 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2020-10-05 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description James Y Knight 2018-11-20 17:31:09 UTC
On both GCC 8.2 and "trunk" on godbolt (https://godbolt.org/z/ykszUZ) given the following program, compiled for x86 -m32, the final GCCAlignOf static assert unexpectedly fails.

However, if you comment out the three StdAlignOf lines, the assert starts passing. Or, if you swap the order such that GCCAlignOf comes first, then it will pass, and the StdAlignOf assert fails, instead. 

It looks like some caching inside GCC is still treating alignof and __alignof as the same thing, even though they now (since the fix for PR69560) are supposed to have differing behavior.

Possibly this is due to both alignof and __alignof mangling to the same character?

===
static_assert(alignof(double) == 4, "");
static_assert(__alignof__(double) == 8, "");

template<typename _Tp, _Tp __v>
struct integral_constant
{
    static constexpr _Tp value = __v;
};

template <class T>
using StdAlignOf = integral_constant<unsigned long, alignof(T)>;

static_assert(StdAlignOf<double>::value == 4, "");

template <class T>
using GCCAlignOf = integral_constant<unsigned long, __alignof__(T)>;

static_assert(GCCAlignOf<double>::value == 8, "");
===
Comment 1 GCC Commits 2020-10-07 14:52:51 UTC
The master branch has been updated by Patrick Palka <ppalka@gcc.gnu.org>:

https://gcc.gnu.org/g:592fe221735bdaa375b1834dd49ce125d0b600d8

commit r11-3704-g592fe221735bdaa375b1834dd49ce125d0b600d8
Author: Patrick Palka <ppalka@redhat.com>
Date:   Wed Oct 7 10:49:00 2020 -0400

    c++: Distinguish alignof and __alignof__ in cp_tree_equal [PR97273]
    
    cp_tree_equal currently considers alignof the same as __alignof__, but
    these operators are semantically different ever since r8-7957.  In the
    testcase below, this causes the second static_assert to fail on targets
    where alignof(double) != __alignof__(double) because the specialization
    table (which uses cp_tree_equal as its equality predicate) conflates the
    two dependent specializations integral_constant<__alignof__(T)> and
    integral_constant<alignof(T)>.
    
    This patch makes cp_tree_equal distinguish between these two operators
    by inspecting the ALIGNOF_EXPR_STD_P flag.
    
    gcc/cp/ChangeLog:
    
            PR c++/88115
            PR libstdc++/97273
            * tree.c (cp_tree_equal) <case ALIGNOF_EXPR>: Return false if
            ALIGNOF_EXPR_STD_P differ.
    
    gcc/testsuite/ChangeLog:
    
            PR c++/88115
            PR libstdc++/97273
            * g++.dg/template/alignof3.C: New test.
Comment 2 GCC Commits 2020-10-08 23:32:59 UTC
The releases/gcc-10 branch has been updated by Patrick Palka <ppalka@gcc.gnu.org>:

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

commit r10-8872-gaeb69dda51e93379fce10fb03ec57650fbf73f31
Author: Patrick Palka <ppalka@redhat.com>
Date:   Thu Oct 8 19:31:57 2020 -0400

    c++: Distinguish alignof and __alignof__ in cp_tree_equal [PR97273]
    
    cp_tree_equal currently considers alignof the same as __alignof__, but
    these operators are semantically different ever since r8-7957.  In the
    testcase below, this causes the second static_assert to fail on targets
    where alignof(double) != __alignof__(double) because the specialization
    table (which uses cp_tree_equal as its equality predicate) conflates the
    two dependent specializations integral_constant<__alignof__(T)> and
    integral_constant<alignof(T)>.
    
    This patch makes cp_tree_equal distinguish between these two operators
    by inspecting the ALIGNOF_EXPR_STD_P flag.
    
    gcc/cp/ChangeLog:
    
            PR c++/88115
            PR libstdc++/97273
            * tree.c (cp_tree_equal) <case ALIGNOF_EXPR>: Return false if
            ALIGNOF_EXPR_STD_P differ.
    
    gcc/testsuite/ChangeLog:
    
            PR c++/88115
            PR libstdc++/97273
            * g++.dg/template/alignof3.C: New test.
    
    (cherry picked from commit 592fe221735bdaa375b1834dd49ce125d0b600d8)
Comment 3 GCC Commits 2020-11-11 20:11:44 UTC
The master branch has been updated by Patrick Palka <ppalka@gcc.gnu.org>:

https://gcc.gnu.org/g:61827d5d9a5a09a8c05d5e41f95b03ebc6c43f61

commit r11-4925-g61827d5d9a5a09a8c05d5e41f95b03ebc6c43f61
Author: Patrick Palka <ppalka@redhat.com>
Date:   Wed Nov 11 14:43:39 2020 -0500

    c++: Correct the handling of alignof(expr) [PR88115]
    
    We're currently neglecting to set the ALIGNOF_EXPR_STD_P flag on an
    ALIGNOF_EXPR when its operand is an expression.  This leads to us
    handling alignof(expr) as if it were written __alignof__(expr), and
    returning the preferred alignment instead of the ABI alignment.  In the
    testcase below, this causes the first and third static_assert to fail on
    x86.
    
    gcc/cp/ChangeLog:
    
            PR c++/88115
            * cp-tree.h (cxx_sizeof_or_alignof_expr): Add bool parameter.
            * decl.c (fold_sizeof_expr): Pass false to
            cxx_sizeof_or_alignof_expr.
            * parser.c (cp_parser_unary_expression): Pass std_alignof to
            cxx_sizeof_or_alignof_expr.
            * pt.c (tsubst_copy): Pass false to cxx_sizeof_or_alignof_expr.
            (tsubst_copy_and_build): Pass std_alignof to
            cxx_sizeof_or_alignof_expr.
            * typeck.c (cxx_alignof_expr): Add std_alignof bool parameter
            and pass it to cxx_sizeof_or_alignof_type.  Set ALIGNOF_EXPR_STD_P
            appropriately.
            (cxx_sizeof_or_alignof_expr): Add std_alignof bool parameter
            and pass it to cxx_alignof_expr.  Assert op is either
            SIZEOF_EXPR or ALIGNOF_EXPR.
    
    libcc1/ChangeLog:
    
            PR c++/88115
            * libcp1plugin.cc (plugin_build_unary_expr): Pass true to
            cxx_sizeof_or_alignof_expr.
    
    gcc/testsuite/ChangeLog:
    
            PR c++/88115
            * g++.dg/cpp0x/alignof6.C: New test.
Comment 4 GCC Commits 2020-11-11 20:11:49 UTC
The master branch has been updated by Patrick Palka <ppalka@gcc.gnu.org>:

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

commit r11-4926-gb1c9b3c3408c1ec8043f9b9e1a148f84bb7f3b25
Author: Patrick Palka <ppalka@redhat.com>
Date:   Wed Nov 11 15:11:23 2020 -0500

    c++: Change the mangling of __alignof__ [PR88115]
    
    This patch changes the mangling of __alignof__ to v111__alignof__,
    making its mangling distinct from that of alignof(type) and
    alignof(expr).
    
    How we mangle ALIGNOF_EXPR now depends on its ALIGNOF_EXPR_STD_P flag,
    which after the previous patch gets consistently set for alignof(type)
    as well as alignof(expr).
    
    gcc/c-family/ChangeLog:
    
            PR c++/88115
            * c-opts.c (c_common_post_options): Update latest_abi_version.
    
    gcc/ChangeLog:
    
            PR c++/88115
            * common.opt (-fabi-version): Document =15.
            * doc/invoke.texi (C++ Dialect Options): Likewise.
    
    gcc/cp/ChangeLog:
    
            PR c++/88115
            * mangle.c (write_expression): Mangle __alignof_ differently
            from alignof when the ABI version is at least 15.
    
    libiberty/ChangeLog:
    
            PR c++/88115
            * cp-demangle.c (d_print_comp_inner)
            <case DEMANGLE_COMPONENT_EXTENDED_OPERATOR>: Don't print the
            "operator " prefix for __alignof__.
            <case DEMANGLE_COMPONENT_UNARY>: Always print parens around the
            operand of __alignof__.
            * testsuite/demangle-expected: Test demangling for __alignof__.
    
    gcc/testsuite/ChangeLog:
    
            PR c++/88115
            * g++.dg/abi/macro0.C: Adjust.
            * g++.dg/cpp0x/alignof7.C: New test.
            * g++.dg/cpp0x/alignof8.C: New test.
Comment 5 GCC Commits 2020-11-13 14:23:24 UTC
The releases/gcc-9 branch has been updated by Patrick Palka <ppalka@gcc.gnu.org>:

https://gcc.gnu.org/g:9df05884b3a30d32744a070d3fc5780b7323231a

commit r9-9043-g9df05884b3a30d32744a070d3fc5780b7323231a
Author: Patrick Palka <ppalka@redhat.com>
Date:   Wed Oct 7 10:49:00 2020 -0400

    c++: Distinguish alignof and __alignof__ in cp_tree_equal [PR97273]
    
    cp_tree_equal currently considers alignof the same as __alignof__, but
    these operators are semantically different ever since r8-7957.  In the
    testcase below, this causes the second static_assert to fail on targets
    where alignof(double) != __alignof__(double) because the specialization
    table (which uses cp_tree_equal as its equality predicate) conflates the
    two dependent specializations integral_constant<__alignof__(T)> and
    integral_constant<alignof(T)>.
    
    This patch makes cp_tree_equal distinguish between these two operators
    by inspecting the ALIGNOF_EXPR_STD_P flag.
    
    gcc/cp/ChangeLog:
    
            PR c++/88115
            PR libstdc++/97273
            * tree.c (cp_tree_equal) <case ALIGNOF_EXPR>: Return false if
            ALIGNOF_EXPR_STD_P differ.
    
    gcc/testsuite/ChangeLog:
    
            PR c++/88115
            PR libstdc++/97273
            * g++.dg/template/alignof3.C: New test.
    
    (cherry picked from commit 592fe221735bdaa375b1834dd49ce125d0b600d8)
Comment 6 James Y Knight 2020-11-15 03:26:02 UTC
>     c++: Change the mangling of __alignof__ [PR88115]

The new mangling chosen for __alignof__(type) seems problematic, and I think this commit ought to be reverted until a new mangling scheme has been chosen.

At the very least, both __alignof__(expression) and __alignof__(type) can't both be encoded as "v111__alignof__", with no indication of whether the argument is a type or an expression, because that's ambiguous.

Possibly it'd be okay to use v112__alignof__z and v112__alignof__t, similar to how "az" and "at" are used for standard alignof expression/type. That solves the ambiguity. But -- this violates the generic grammar, which doesn't allow for a vendor operator to take type arguments at all.
  <operator-name> ::= v <digit> <source-name>
  <expression> ::= <unary operator-name> <expression>

Now...possibly it's okay to violate that and pass a type, regardless. But, then the demangler needs to know what every vendor extension operator's semantics are in order to demangle -- which seems against the intent of the standardized "vendor operator" syntax. So, that's not great.

This seems like a problem which may be worth solving properly and generally in the itanium ABI -- maybe now's the time to actually do this? (Fixing this would allow __typeof__, __builtin_convertvector, and __builtin_offsetof to be mangleable too...) See also PR13740 and PR11078, for earlier (MUCH earlier!) discussion of possible mangling syntax extensions for this sort of thing. Unfortunately, it never went anywhere...

ISTM that the most promising proposal in those earlier threads was to add a "Y" to both type and expression expansions.

To support vendor-specific builtins which returns a type, and can take types or expressions as arguments (e.g. typeof), add:
  <type> ::= Y <sourcename> <template-arg>+ E

To support vendor-specific builtins which return a value, and can take types or expressions as arguments (e.g. __builtin_offsetof, or __alignof__):
  <expression> ::= Y <sourcename> <template-arg>+ E
Comment 7 James Y Knight 2020-11-17 01:23:49 UTC
I've created an ABI proposal against itanium abi, here:
https://github.com/itanium-cxx-abi/cxx-abi/issues/112

I realized since writing my last comment here that
  <type> ::= u <source-name> [<template-args>] # vendor extended type
has been added a few months ago. Therefore, nothing needs to be added to support extended expressions as <type>.

And, therefore, since 'u' is also free in expression, I've proposed to simply add the same 'u' syntax to "expression", rather than using "Y", as per previous comment.

That is:
  <expression> ::= u <source-name> [<template-args>] # vendor extended expression
Comment 8 Jonathan Wakely 2020-11-17 14:14:41 UTC
Thanks for starting the discussion there.
Comment 9 James Y Knight 2021-01-07 23:13:44 UTC
Proposed patch posted for the itanium-cxx-abi:
 https://github.com/itanium-cxx-abi/cxx-abi/pull/115/files
using the syntax:
 u <source-name> <template-arg>* E

And to Clang, to use that syntax:
 https://reviews.llvm.org/D93922

I hope both GCC and Clang can do the same thing here.
Comment 10 James Y Knight 2021-01-29 17:57:03 UTC
Seeing as GCC is now in Stage 4 before the next release, I'd like to ping this bug.

Should the priority be upgraded? I think fixing this so that GCC doesn't use 'v111__alignof__' should be considered a release-blocker: either the mangling change should be reverted to the previous behavior, or fixed.

FWIW, the change to clang has been committed, and is planned to be in the upcoming Clang 12 release (https://reviews.llvm.org/rG9c7aeaebb3ac1b94200b59b111742cb6b8f090c2)
Comment 11 Patrick Palka 2021-02-01 14:16:18 UTC
(In reply to James Y Knight from comment #10)
> Seeing as GCC is now in Stage 4 before the next release, I'd like to ping
> this bug.
> 
> Should the priority be upgraded? I think fixing this so that GCC doesn't use
> 'v111__alignof__' should be considered a release-blocker: either the
> mangling change should be reverted to the previous behavior, or fixed.

Yes, thanks for the ping.  I'll work on a patch for GCC 11 to follow suit, so that Clang and GCC do the same thing.

> 
> FWIW, the change to clang has been committed, and is planned to be in the
> upcoming Clang 12 release
> (https://reviews.llvm.org/rG9c7aeaebb3ac1b94200b59b111742cb6b8f090c2)
Comment 12 GCC Commits 2021-03-31 02:58:40 UTC
The master branch has been updated by Patrick Palka <ppalka@gcc.gnu.org>:

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

commit r11-7921-ga3bf6ce7f2e17f2c977c13df23eb718e7b433dcd
Author: Patrick Palka <ppalka@redhat.com>
Date:   Tue Mar 30 22:57:11 2021 -0400

    c++: Adjust mangling of __alignof__ [PR88115]
    
    r11-4926 made __alignof__ get mangled differently from alignof,
    encoding __alignof__ as a vendor extended operator.  But this
    mangling is problematic for the reasons mentioned in
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88115#c6.
    
    This patch changes our mangling of __alignof__ to instead use the
    new "vendor extended expression" syntax that's proposed in
    https://github.com/itanium-cxx-abi/cxx-abi/issues/112.  Clang does
    the same thing already, so after this patch Clang and GCC agree
    about the mangling of __alignof__(type) and __alignof__(expr).
    
    gcc/cp/ChangeLog:
    
            PR c++/88115
            * mangle.c (write_expression): Adjust the mangling of
            __alignof__.
    
    include/ChangeLog:
    
            PR c++/88115
            * demangle.h (enum demangle_component_type): Add
            DEMANGLE_COMPONENT_VENDOR_EXPR.
    
    libiberty/ChangeLog:
    
            PR c++/88115
            * cp-demangle.c (d_dump, d_make_comp, d_expression_1)
            (d_count_templates_scopes): Handle DEMANGLE_COMPONENT_VENDOR_EXPR.
            (d_print_comp_inner): Likewise.
            <case DEMANGLE_COMPONENT_EXTENDED_OPERATOR>: Revert r11-4926
            change.
            <case DEMANGLE_COMPONENT_UNARY>: Likewise.
            * testsuite/demangle-expected: Adjust __alignof__ tests.
    
    gcc/testsuite/ChangeLog:
    
            PR c++/88115
            * g++.dg/cpp0x/alignof7.C: Adjust expected mangling.
Comment 13 GCC Commits 2021-04-21 12:08:14 UTC
The releases/gcc-8 branch has been updated by Patrick Palka <ppalka@gcc.gnu.org>:

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

commit r8-10855-ga34c19926b87f9cadd5ea84f5c5b93ae76b14558
Author: Patrick Palka <ppalka@redhat.com>
Date:   Wed Oct 7 10:49:00 2020 -0400

    c++: Distinguish alignof and __alignof__ in cp_tree_equal [PR97273]
    
    cp_tree_equal currently considers alignof the same as __alignof__, but
    these operators are semantically different ever since r8-7957.  In the
    testcase below, this causes the second static_assert to fail on targets
    where alignof(double) != __alignof__(double) because the specialization
    table (which uses cp_tree_equal as its equality predicate) conflates the
    two dependent specializations integral_constant<__alignof__(T)> and
    integral_constant<alignof(T)>.
    
    This patch makes cp_tree_equal distinguish between these two operators
    by inspecting the ALIGNOF_EXPR_STD_P flag.
    
    gcc/cp/ChangeLog:
    
            PR c++/88115
            PR libstdc++/97273
            * tree.c (cp_tree_equal) <case ALIGNOF_EXPR>: Return false if
            ALIGNOF_EXPR_STD_P differ.
    
    gcc/testsuite/ChangeLog:
    
            PR c++/88115
            PR libstdc++/97273
            * g++.dg/template/alignof3.C: New test.
    
    (cherry picked from commit 592fe221735bdaa375b1834dd49ce125d0b600d8)
Comment 14 Patrick Palka 2021-04-21 12:10:41 UTC
Fixed.