Bug 94050 - [10 Regression] C++ ABI change on armv7hl-linux-gnueabi since r10-1302
Summary: [10 Regression] C++ ABI change on armv7hl-linux-gnueabi since r10-1302
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 10.0
: P1 normal
Target Milestone: 10.0
Assignee: Marek Polacek
URL:
Keywords: ABI, patch
Depends on:
Blocks:
 
Reported: 2020-03-05 14:29 UTC by Jakub Jelinek
Modified: 2020-03-09 14:34 UTC (History)
2 users (show)

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


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2020-03-05 14:29:06 UTC
Since r10-1302-gc3337b44c40dd1545e00034cb8e1ae1c0dae0fa6
the following test FAILs on armv7hl-linux-gnueabi (but not on i686 or x86_64).
This breaks at least mozjs68 build:

struct alignas(8) Cell {};
struct TenuredCell : public Cell {};
struct BaseShape : public TenuredCell {
  void *p;
  unsigned q, r;
  void *s;
  __UINTPTR_TYPE__ t;
};
static_assert (sizeof (BaseShape) % 8 == 0, "");
Comment 1 Marek Polacek 2020-03-05 14:58:39 UTC
Here's the thread where the patch originated: https://gcc.gnu.org/ml/gcc-patches/2019-06/msg01189.html
Comment 2 Marek Polacek 2020-03-05 15:44:48 UTC
Looks like we're losing the TYPE_USER_ALIGN bit.  That's probably because arm is STRICT_ALIGNMENT and so finalize_type_size does this:

1930       /* Don't override a larger alignment requirement coming from a user
1931          alignment of one of the fields.  */
1932       if (mode_align >= TYPE_ALIGN (type))
1933         {
1934           SET_TYPE_ALIGN (type, mode_align);
1935           TYPE_USER_ALIGN (type) = 0;
1936         }
Comment 3 Marek Polacek 2020-03-05 17:51:03 UTC
But that happens even before r10-1302-gc3337b44c40dd1545e00034cb8e1ae1c0dae0fa6.

The actual problem is that in layout_class_type for TenuredCell we see that the size of TenuredCell and its CLASSTYPE_AS_BASE match, so we set
CLASSTYPE_AS_BASE (t) = t;

But while TYPE_USER_ALIGN of TenuredCell was 0, TYPE_USER_ALIGN of its CLASSTYPE_AS_BASE was 1.  After we replace it, it's no longer 1.

So then we perform layout_empty_base_or_field for TenuredCell.  Since TYPE_USER_ALIGN of its CLASSTYPE_AS_BASE is now 0, we don't do this:

  if (CLASSTYPE_USER_ALIGN (type))
    {
      rli->record_align = MAX (rli->record_align, CLASSTYPE_ALIGN (type));
      if (warn_packed)
        rli->unpacked_align = MAX (rli->unpacked_align, CLASSTYPE_ALIGN (type));
      TYPE_USER_ALIGN (rli->t) = 1;
    }

where rli->t is BaseShape.  And that's how we lose the alignas info on BaseShape.  Then sizeof thinks its size is 20B and it's not aligned to 24B.

CLASSTYPE_USER_ALIGN is defined as TYPE_USER_ALIGN (CLASSTYPE_AS_BASE (NODE)).
Comment 4 Marek Polacek 2020-03-05 18:15:44 UTC
So quite obviously *a* fix would be

--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -6705,6 +6705,7 @@ layout_class_type (tree t, tree *virtuals_p)
 
   /* If we didn't end up needing an as-base type, don't use it.  */
   if (CLASSTYPE_AS_BASE (t) != t
+      && !CLASSTYPE_USER_ALIGN (t)
       && tree_int_cst_equal (TYPE_SIZE (t),
                 TYPE_SIZE (CLASSTYPE_AS_BASE (t))))
     CLASSTYPE_AS_BASE (t) = t;

(needs a comment of course).  Since the original fix was to avoid creating extra copies for LTO purposes, this slight degradation should not be too bad?
Comment 5 Jakub Jelinek 2020-03-09 14:22:43 UTC
commit r10-7089-g8475f2902a2e2ca5f7ace8bc5265bd1a815dda20
Author: Marek Polacek <polacek@redhat.com>
Date:   Thu Mar 5 14:07:25 2020 -0500

    c++: Fix ABI issue with alignas on armv7hl [PR94050]
    
    The static_assert in the following test was failing on armv7hl because
    we were disregarding the alignas specifier on Cell.  BaseShape's data
    takes up 20B on 32-bit architectures, but we failed to round up its
    TYPE_SIZE.  This happens since the
    <https://gcc.gnu.org/ml/gcc-patches/2019-06/msg01189.html>
    patch: here, in layout_class_type for TenuredCell, we see that the size
    of TenuredCell and its CLASSTYPE_AS_BASE match, so we set
    
      CLASSTYPE_AS_BASE (t) = t;
    
    While TYPE_USER_ALIGN of TenuredCell was 0, because finalize_type_size
    called from finish_record_layout reset it, TYPE_USER_ALIGN of its
    CLASSTYPE_AS_BASE still remained 1.  After we replace it, it's no longer
    1.  Then we perform layout_empty_base_or_field for TenuredCell and since
    TYPE_USER_ALIGN of its CLASSTYPE_AS_BASE is now 0, we don't do this
    adjustment:
    
      if (CLASSTYPE_USER_ALIGN (type))
        {
          rli->record_align = MAX (rli->record_align, CLASSTYPE_ALIGN (type));
          if (warn_packed)
            rli->unpacked_align = MAX (rli->unpacked_align, CLASSTYPE_ALIGN (type));
          TYPE_USER_ALIGN (rli->t) = 1;
        }
    
    where rli->t is BaseShape.  Then finalize_record_size won't use the
    correct rli->record_align and therefore
      /* Round the size up to be a multiple of the required alignment.  */
      TYPE_SIZE (rli->t) = round_up (unpadded_size, TYPE_ALIGN (rli->t));
    after this we end up with the wrong size.
    
    Since the original fix was to avoid creating extra copies for LTO
    purposes, I think the following fix should be acceptable.
    
            PR c++/94050 - ABI issue with alignas on armv7hl.
            * class.c (layout_class_type): Don't replace a class's
            CLASSTYPE_AS_BASE if their TYPE_USER_ALIGN don't match.
    
            * g++.dg/abi/align3.C: New test.
Comment 6 Marek Polacek 2020-03-09 14:34:51 UTC
Fixed.