[PATCH] aarch64: Tighten up checks for ubfix [PR98681]

2021-01-23 Thread Jakub Jelinek via Gcc-patches
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

2021-01-23 Thread Jakub Jelinek via Gcc-patches
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

2021-01-23 Thread Paul Richard Thomas via Gcc-patches
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

2021-01-23 Thread Segher Boessenkool
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

2021-01-23 Thread Iain Buclaw via Gcc-patches
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