[PATCH] aarch64: Tighten up checks for ubfix [PR98681]
Hi! The testcase in the patch doesn't assemble, because the instruction requires that the penultimate operand (lsb) range is [0, 32] (or [0, 64]) and the last operand's range is [1, 32 - lsb] (or [1, 64 - lsb]). The INTVAL (shft_amnt) < GET_MODE_BITSIZE (mode) will accept the lsb operand to be in range [MIN, 32] (or [MIN, 64]) and then we invoke UB in the compiler and sometimes it will make it through. The first patch just changes that check to UINTVAL (shft_amnt) < GET_MODE_BITSIZE (mode) so that it will return false for negative shift amounts, thus there won't be UB in the compiler later. The second patch changes all the INTVAL uses in that function to UINTVAL, which isn't strictly necessary, but can be done (e.g. after the UINTVAL (shft_amnt) < GET_MODE_BITSIZE (mode) check we know it is not negative and thus INTVAL (shft_amnt) and UINTVAL (shft_amnt) then behave the same. But, I had to add INTVAL (mask) > 0 check in that case, otherwise we risk (hypothetically) emitting instruction that doesn't assemble. The problem is with masks that have the MSB bit set, while the instruction can handle those, e.g. ubfiz w1, w0, 13, 19 will do (w0 << 13) & 0xe000 in RTL we represent SImode constants with MSB set as negative HOST_WIDE_INT, so it will actually be HOST_WIDE_INT_C (0xe000), and the instruction uses %P3 to print the last operand, which calls asm_fprintf (f, "%u", popcount_hwi (INTVAL (x))) to print that. But that will not print 19, but 51 instead, will include there also all the copies of the sign bit. Not supporting those masks with MSB set isn't a big loss though, they really shouldn't appear normally, as both GIMPLE and RTL optimizations should optimize those away (one isn't masking any bits off with such masks, so just w0 << 13 will do too). Both patches were successfully bootstrapped/regtested on aarch64-linux. Ok for trunk and which one? Jakub 2021-01-23 Jakub Jelinek PR target/98681 * config/aarch64/aarch64.c (aarch64_mask_and_shift_for_ubfiz_p): Compare UINTVAL (shft_amnt) against GET_MODE_BITSIZE (mode) rather than INTVAL (shft_amnt). * gcc.c-torture/execute/pr98681.c: New test. --- gcc/config/aarch64/aarch64.c.jj 2021-01-13 11:36:27.069888393 +0100 +++ gcc/config/aarch64/aarch64.c2021-01-22 16:43:14.791305939 +0100 @@ -12060,7 +12060,7 @@ aarch64_mask_and_shift_for_ubfiz_p (scal rtx shft_amnt) { return CONST_INT_P (mask) && CONST_INT_P (shft_amnt) -&& INTVAL (shft_amnt) < GET_MODE_BITSIZE (mode) +&& UINTVAL (shft_amnt) < GET_MODE_BITSIZE (mode) && exact_log2 ((INTVAL (mask) >> INTVAL (shft_amnt)) + 1) >= 0 && (INTVAL (mask) & ((HOST_WIDE_INT_1U << INTVAL (shft_amnt)) - 1)) == 0; --- gcc/testsuite/gcc.c-torture/execute/pr98681.c.jj2021-01-22 16:45:05.102070501 +0100 +++ gcc/testsuite/gcc.c-torture/execute/pr98681.c 2021-01-22 16:44:34.165416961 +0100 @@ -0,0 +1,18 @@ +/* PR target/98681 */ + +__attribute__((noipa)) int +foo (int x) +{ + if (x > 32) +return (x << -64) & 255; + else +return x; +} + +int +main () +{ + if (foo (32) != 32 || foo (-150) != -150) +__builtin_abort (); + return 0; +} 2021-01-23 Jakub Jelinek PR target/98681 * config/aarch64/aarch64.c (aarch64_mask_and_shift_for_ubfiz_p): Use UINTVAL (shft_amnt) and UINTVAL (mask) instead of INTVAL (shft_amnt) and INTVAL (mask). Add && INTVAL (mask) > 0 condition. * gcc.c-torture/execute/pr98681.c: New test. --- gcc/config/aarch64/aarch64.c.jj 2021-01-13 11:36:27.069888393 +0100 +++ gcc/config/aarch64/aarch64.c2021-01-22 18:53:18.611518461 +0100 @@ -12060,10 +12060,11 @@ aarch64_mask_and_shift_for_ubfiz_p (scal rtx shft_amnt) { return CONST_INT_P (mask) && CONST_INT_P (shft_amnt) -&& INTVAL (shft_amnt) < GET_MODE_BITSIZE (mode) -&& exact_log2 ((INTVAL (mask) >> INTVAL (shft_amnt)) + 1) >= 0 -&& (INTVAL (mask) -& ((HOST_WIDE_INT_1U << INTVAL (shft_amnt)) - 1)) == 0; +&& INTVAL (mask) > 0 +&& UINTVAL (shft_amnt) < GET_MODE_BITSIZE (mode) +&& exact_log2 ((UINTVAL (mask) >> UINTVAL (shft_amnt)) + 1) >= 0 +&& (UINTVAL (mask) +& ((HOST_WIDE_INT_1U << UINTVAL (shft_amnt)) - 1)) == 0; } /* Return true if the masks and a shift amount from an RTX of the form --- gcc/testsuite/gcc.c-torture/execute/pr98681.c.jj2021-01-22 16:45:05.102070501 +0100 +++ gcc/testsuite/gcc.c-torture/execute/pr98681.c 2021-01-22 16:44:34.165416961 +0100 @@ -0,0 +1,18 @@ +/* PR target/98681 */ + +__attribute__((noipa)) int +foo (int x) +{ + if (x > 32) +return (x << -64) & 255; + else +return x; +} + +int +main () +{ + if (foo (32) != 32 || foo (-150) != -150) +__builtin_abort (); + return 0; +}
Re: [PATCH] testsuite: Fix sse2-andnpd-1.c and sse-andnps-1.c testscases on powerpc
On Fri, Jan 22, 2021 at 06:56:37PM -0600, Segher Boessenkool wrote: > On Fri, Jan 22, 2021 at 08:02:28PM +0100, Jakub Jelinek wrote: > > On Mon, Sep 21, 2020 at 10:12:20AM +0200, Richard Biener wrote: > > > On Mon, 21 Sep 2020, Jan Hubicka wrote: > > > > these testcases now fails because they contains an invalid type puning > > > > that happens via const VALUE_TYPE *v pointer. Since the check function > > > > is noinline, modref is needed to trigger the wrong code. > > > > I think it is easiest to fix it by no-strict-aliasing. > > > > > diff --git a/gcc/testsuite/gcc.target/i386/m128-check.h > > > > b/gcc/testsuite/gcc.target/i386/m128-check.h > > > > index 48b23328539..6f414b07be7 100644 > > > > --- a/gcc/testsuite/gcc.target/i386/m128-check.h > > > > +++ b/gcc/testsuite/gcc.target/i386/m128-check.h > > > > @@ -78,6 +78,7 @@ typedef union > > > > > > > > #define CHECK_EXP(UINON_TYPE, VALUE_TYPE, FMT) \ > > > > static int \ > > > > +__attribute__((optimize ("no-strict-aliasing"))) \ > > > > __attribute__((noinline, unused)) \ > > > > check_##UINON_TYPE (UINON_TYPE u, const VALUE_TYPE *v) \ > > > > { \ > > > > On powerpc64le the tests suffer from the exact same issue. > > > > Tested on powerpc64le-linux, ok for trunk? > > So what is the actual error here? This whole union stuff is because we > *do* want proper aliasing, afaics. The reading through union is not where the problem is, the problem was in the reading through VALUE_TYPE, because the testcase does: int e[4]; ... e[0] = (~source1[0]) & source2[0]; e[1] = (~source1[1]) & source2[1]; e[2] = (~source1[2]) & source2[2]; e[3] = (~source1[3]) & source2[3]; if (check_union128 (u, (float *)e)) So check_union128 reads through VALUE_TYPE of float, but the variable has dynamic type int[4]. Even making a e union of int[4] and float[4] and passing address of the float field in it wouldn't be correct, but e.g. having another variable with float[4] type and memcpying e into it would work too. So: int e[4]; float f[4]; ... e[0] = (~source1[0]) & source2[0]; e[1] = (~source1[1]) & source2[1]; e[2] = (~source1[2]) & source2[2]; e[3] = (~source1[3]) & source2[3]; memcpy (f, e, sizeof (e)); if (check_union128 (u, f)) The reason I chose the "no-strict-aliasing" attribute (and already committed based on Richi's ack) was consistency with the i386 testcase. I can change both. Jakub
[Patch, fortran] PR98573 - Dynamic type lost on assignment
This is a relatively obvious patch. The chunk in trans-array.c is not part of the fix for the PR but does suppress some of the bad dtype's that arise from allocation of class objects. The part in trans-stmt.c provides vptrs for all class allocations if the expression3 is available. Regtests on FC33/x86_64 Paul Fortran: Fix missing setting of vptrs in allocate statements [PR98573]. 2021-01-22 Paul Thomas gcc/fortran PR fortran/98573 * trans-array.c (gfc_array_init_size): If expr3 descriptor is present, use it for the type. * trans-stmt.c (gfc_trans_allocate): Use the expr3 vptr for all class allocations. gcc/testsuite/ PR fortran/98573 * gfortran.dg/associated_target_7.f90 : New test. diff --git a/gcc/fortran/trans-array.c b/gcc/fortran/trans-array.c index 4bd4db877bd..306c2de7be7 100644 --- a/gcc/fortran/trans-array.c +++ b/gcc/fortran/trans-array.c @@ -5540,7 +5540,13 @@ gfc_array_init_size (tree descriptor, int rank, int corank, tree * poffset, gfc_se se; int n; - type = TREE_TYPE (descriptor); + if (expr->ts.type == BT_CLASS + && expr3_desc != NULL_TREE + && GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (expr3_desc))) +type = TREE_TYPE (expr3_desc); + else +type = TREE_TYPE (descriptor); + stride = gfc_index_one_node; offset = gfc_index_zero_node; diff --git a/gcc/fortran/trans-stmt.c b/gcc/fortran/trans-stmt.c index 547468f7648..2bd7fdf0f1c 100644 --- a/gcc/fortran/trans-stmt.c +++ b/gcc/fortran/trans-stmt.c @@ -6908,7 +6908,8 @@ gfc_trans_allocate (gfc_code * code) /* Set the vptr only when no source= is set. When source= is set, then the trans_assignment below will set the vptr. */ - if (al_vptr != NULL_TREE && (!code->expr3 || code->expr3->mold)) + if (al_vptr != NULL_TREE && (!code->expr3 || code->expr3->mold + || code->expr3->ts.type == BT_CLASS)) { if (expr3_vptr != NULL_TREE) /* The vtab is already known, so just assign it. */ ! { dg-do run } ! ! Tests the fix of PR98573. Fixed missing vptrs for class allocations. ! ! Contributed by Davis Asanza ! module counts integer :: integer_count = 0 integer :: other_count = 0 integer :: alloc_counts = 0 end module counts module foo1 use counts type, public:: box class(*), allocatable :: val(:) end type contains subroutine store1(this, val) class(box), intent(out) :: this class(*), intent(in) :: val(:) this%val = val end subroutine store1 subroutine store2(this, val) class(box), intent(out) :: this class(*), intent(in) :: val(:) allocate(this%val, source=val) end subroutine store2 subroutine vector_type(val) class(*), intent(in) :: val(:) select type (val) type is (integer) integer_count = integer_count + 1 class default other_count = other_count + 1 end select end subroutine vector_type end module foo1 module foo2 use counts contains subroutine store1(arr, val) class(*), allocatable, intent(out) :: arr(:) class(*), intent(in) :: val(:) arr = val end subroutine store1 subroutine store2(arr, val) class(*), allocatable, intent(out) :: arr(:) class(*), intent(in) :: val(:) allocate(arr, source=val) end subroutine store2 end module foo2 module foo3 use counts type, public:: box class(*), allocatable :: val(:) end type contains subroutine store1(this, val) class(box), intent(out) :: this class(*), intent(in) :: val(:) this%val = val end subroutine store1 subroutine store2(this, val) class(box), intent(out) :: this class(*), intent(in) :: val(:) allocate(this%val, source=val) end subroutine store2 subroutine vector_type(val) class(*), intent(in) :: val(:) select type (val) type is (integer) integer_count = integer_count + 1 class default other_count = other_count + 1 end select end subroutine vector_type end module foo3 program prog use counts implicit none call bar1 ! Test the original problem call bar2 ! Test comment 1 call bar3 ! Test comment 3 if (integer_count .ne. 6) stop 1 if (other_count .ne. 0) stop 2 if (alloc_counts .ne. 2) stop 3 contains subroutine bar1 use foo1 type(box) :: b call store1(b, [1, 2, 3]) call vector_type(b%val) ! OTHER call store2(b, [1, 2, 3]) call vector_type(b%val) ! INTEGER end subroutine bar1 subroutine bar2 use foo2 class(*), allocatable :: arr(:) call store1(arr, [1, 2, 3]) ! SEGFAULT select type (a => arr) type is (integer) if (all (a .eq. [1, 2, 3])) alloc_counts = alloc_counts + 1 end select deallocate (arr) call store2(arr, [1, 2, 3]) ! NO PROBLEM select type (a => arr) type is (integer) if (all (a .eq. [1, 2, 3])) alloc_counts = alloc_counts + 1 end select end subroutine bar2 subroutine bar3 use foo3 type(box) :: b integer, allocatable :: arr1(:) integer, dimension(0) :: arr2 allocate(arr1(0)) call store1
Re: [PATCH] testsuite: Fix sse2-andnpd-1.c and sse-andnps-1.c testscases on powerpc
Hi! On Sat, Jan 23, 2021 at 09:41:23AM +0100, Jakub Jelinek wrote: > On Fri, Jan 22, 2021 at 06:56:37PM -0600, Segher Boessenkool wrote: > > So what is the actual error here? This whole union stuff is because we > > *do* want proper aliasing, afaics. > > The reading through union is not where the problem is, the problem > was in the reading through VALUE_TYPE, because the testcase does: > int e[4]; > ... > e[0] = (~source1[0]) & source2[0]; > e[1] = (~source1[1]) & source2[1]; > e[2] = (~source1[2]) & source2[2]; > e[3] = (~source1[3]) & source2[3]; > > if (check_union128 (u, (float *)e)) Right, that should use such a union as well, or similar. Most of the other similar mechanics in the testcase are because we *do* want proper alising rules, so it would be a shame to throw it all away now. > So check_union128 reads through VALUE_TYPE of float, but the variable > has dynamic type int[4]. Even making a e union of int[4] and float[4] > and passing address of the float field in it wouldn't be correct, > but e.g. having another variable with float[4] type and memcpying > e into it would work too. So: > int e[4]; > float f[4]; > ... > e[0] = (~source1[0]) & source2[0]; > e[1] = (~source1[1]) & source2[1]; > e[2] = (~source1[2]) & source2[2]; > e[3] = (~source1[3]) & source2[3]; > memcpy (f, e, sizeof (e)); > > if (check_union128 (u, f)) That, or have the function take a pointer-to-union, or make "e" a union itself. All should work. > The reason I chose the "no-strict-aliasing" attribute (and already > committed based on Richi's ack) was consistency with the i386 > testcase. I can change both. Yes please. Thanks! Segher
[committed] libphobos: Fix executables segfault on mipsel architecture
Hi, This patch fixes an issue running programs linked to the shared libphobos library on MIPS. The dynamic section on MIPS is read-only, but this was not properly handled in the runtime library. Bootstrapped and regression tested on mipsel-linux-gnu, and committed to mainline with backports to the releases/gcc-10 and gcc-9 branches. Regards Iain --- libphobos/ChangeLog: PR d/98806 * libdruntime/gcc/sections/elf_shared.d (MIPS_Any): Declare version for MIPS32 and MIPS64. (getDependencies): Adjust dlpi_addr on MIPS_Any. --- libphobos/libdruntime/gcc/sections/elf_shared.d | 4 1 file changed, 4 insertions(+) diff --git a/libphobos/libdruntime/gcc/sections/elf_shared.d b/libphobos/libdruntime/gcc/sections/elf_shared.d index 9050413b261..427759a4f94 100644 --- a/libphobos/libdruntime/gcc/sections/elf_shared.d +++ b/libphobos/libdruntime/gcc/sections/elf_shared.d @@ -22,6 +22,8 @@ module gcc.sections.elf_shared; +version (MIPS32) version = MIPS_Any; +version (MIPS64) version = MIPS_Any; version (RISCV32) version = RISCV_Any; version (RISCV64) version = RISCV_Any; version (S390)version = IBMZ_Any; @@ -763,6 +765,8 @@ version (Shared) // in glibc: #define DL_RO_DYN_SECTION 1 version (RISCV_Any) strtab = cast(const(char)*)(info.dlpi_addr + dyn.d_un.d_ptr); // relocate +else version (MIPS_Any) +strtab = cast(const(char)*)(info.dlpi_addr + dyn.d_un.d_ptr); // relocate else strtab = cast(const(char)*)dyn.d_un.d_ptr; } -- 2.27.0