Re: Walk pointer_to and reference_to chain in free_lang_data

[Richard Biener]

On Tue, 21 Aug 2018, Jan Hubicka wrote:

> Hi,
> extra sanity checking I am going to send in followup patch noticed that we
> stream pointer types that was never seen by free_lang_data.  This is because
> they are referenced by TYPE_POINTER_TO list and are inserted into the gimple
> statements later when we wrap everything in MEM_REF (by make_pointer_type).
> 
> Bootstrapped/regtested x86_64-linux, OK?

Hmm, but make_pointer_type might as well create new pointer types
that didn't exist before - shouldn't those have the same problem
(which actually is?)?  Note the pointed-to types are already in the IL,
so another option would be to create (and walk) the pointer types
under the same circumstance as streaming creates them when walking the
IL.  Note that new pointer types can be created between free-lang-data
and streaming anyways ...

So again - what's the real reason?  I bet the sanity checking might
trip over late creation of pointer-types so I'd rather fix the
sanity checking and only check the pointed-to types were visited?

Richard.

> Honza
> 
>   * tree.c (find_decls_types_r): Walk also TYPE_NEXT_PTR_TO and
>   TYPE_NEXT_REF_TO.
> Index: tree.c
> ===
> --- tree.c(revision 263699)
> +++ tree.c(working copy)
> @@ -5525,9 +5525,14 @@ find_decls_types_r (tree *tp, int *ws, v
>fld_worklist_push (TYPE_POINTER_TO (t), fld);
>fld_worklist_push (TYPE_REFERENCE_TO (t), fld);
>fld_worklist_push (TYPE_NAME (t), fld);
> -  /* Do not walk TYPE_NEXT_PTR_TO or TYPE_NEXT_REF_TO.  We do not stream
> -  them and thus do not and want not to reach unused pointer types
> -  this way.  */
> +  /* While we do not stream TYPE_POINTER_TO and TYPE_REFERENCE_TO
> +  lists, we may look types up in these lists and use them while
> +  optimizing the function body.  Thus we need to free lang data
> +  in them.  */
> +  if (TREE_CODE (t) == POINTER_TYPE)
> +fld_worklist_push (TYPE_NEXT_PTR_TO (t), fld);
> +  if (TREE_CODE (t) == REFERENCE_TYPE)
> +fld_worklist_push (TYPE_NEXT_REF_TO (t), fld);
>if (!POINTER_TYPE_P (t))
>   fld_worklist_push (TYPE_MIN_VALUE_RAW (t), fld);
>/* TYPE_MAX_VALUE_RAW is TYPE_BINFO for record types.  */
> 
> 

-- 
Richard Biener 
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 
21284 (AG Nuernberg)

Re: Walk pointer_to and reference_to chain in free_lang_data

[Jan Hubicka]

> On Tue, 21 Aug 2018, Jan Hubicka wrote:
> 
> > Hi,
> > extra sanity checking I am going to send in followup patch noticed that we
> > stream pointer types that was never seen by free_lang_data.  This is because
> > they are referenced by TYPE_POINTER_TO list and are inserted into the gimple
> > statements later when we wrap everything in MEM_REF (by make_pointer_type).
> > 
> > Bootstrapped/regtested x86_64-linux, OK?
> 
> Hmm, but make_pointer_type might as well create new pointer types
> that didn't exist before - shouldn't those have the same problem
> (which actually is?)?  Note the pointed-to types are already in the IL,

I am now clearing TYPE_STUB_DECL to NULL and check that it is NULL later in
streaming.  There are indeed some other cases where this check triggers.

Pointer types are not problem because they are created w/o stubs.  Other issues
I see with this are backend produced structures - gcov, asan and ubsan all
create structures with non-NULL stubs.  They are DECL_ARTIFICIAL so I am not
quite sure why we set stubs there.

  tree type_decl = build_decl (input_location, TYPE_DECL,   
   get_identifier ("__asan_global"), ret);  
  DECL_IGNORED_P (type_decl) = 1;   
  DECL_ARTIFICIAL (type_decl) = 1;  
  TYPE_FIELDS (ret) = fields[0];
  TYPE_NAME (ret) = type_decl;  
  TYPE_STUB_DECL (ret) = type_decl; 

It seems to me that setting TYPE_NAME to identifier node would be easier.
gcov uses finish_builtin_struct which in turn does:

#if 0 /* not yet, should get fixed properly later */
  TYPE_NAME (type) = make_type_decl (get_identifier (name), type);
#else
  TYPE_NAME (type) = build_decl (BUILTINS_LOCATION,
 TYPE_DECL, get_identifier (name), type);
#endif
  TYPE_STUB_DECL (type) = TYPE_NAME (type);

It does not seem to set artificial.  I am not quite sure what to do about
those.

In any case we should free lang data frontend created pointer types that may
get back to IL stream.  C++ will give them TYPE_DECL based TYPE_NAMEs that
we want to translate to identifiers for example.

Honza

Re: [PATCH] Add a character size parameter to c_strlen/get_range_strlen

[Richard Biener]

On Wed, 22 Aug 2018, Bernd Edlinger wrote:

> On 08/21/18 10:59, Richard Biener wrote:
> > On Tue, 21 Aug 2018, Bernd Edlinger wrote:
> > 
> >> gcc -S -O2 -Wall -Wformat-overflow -ftrack-macro-expansion=0 -fshort-wchar 
> >> builtin-sprintf-warn-20.c
> >> builtin-sprintf-warn-20.c: In function 'test':
> >> builtin-sprintf-warn-20.c:19:39: warning: hex escape sequence out of range
> >> 19 | ? (char*)L"\x4142\x4344" : (char*)L"\x41424344\x45464748";
> >>  |   ^~~
> >>
> >> Hmm, this test might create some noise on short-wchar targets.
> >>
> >> I would prefer a warning here, about the wrong type of the parameter.
> >> The buffer overflow is only a secondary thing.
> >>
> >> For constant objects like those, the GIMPLE type is still guaranteed to be 
> >> reliable,
> >> right?
> > 
> > TREE_TYPE of tcc_declaration and tcc_constant trees should more-or-less
> > (minus qualifications not affecting semantics) be those set by
> > frontends.
> > 
> 
> and in this case:
> 
> const union
> { struct {
>  wchar_t x[4];
>};
>struct {
>  char z[8];
>};
> } u = {{L"123"}};
> 
> int test()
> {
>return __builtin_strlen(u.z);
> }
> 
> 
> string_constant works out the initializer for u.x
> which has a different type than u.z

Yes.  That's because it uses ctor-for-folding and friends.  It's
a question of the desired semantics of string_constant whether
it should better return NULL_TREE in this case or whether the
caller has to deal with type mismatches.

Richard.

Re: Walk pointer_to and reference_to chain in free_lang_data

[Richard Biener]

On Wed, 22 Aug 2018, Jan Hubicka wrote:

> > On Tue, 21 Aug 2018, Jan Hubicka wrote:
> > 
> > > Hi,
> > > extra sanity checking I am going to send in followup patch noticed that we
> > > stream pointer types that was never seen by free_lang_data.  This is 
> > > because
> > > they are referenced by TYPE_POINTER_TO list and are inserted into the 
> > > gimple
> > > statements later when we wrap everything in MEM_REF (by 
> > > make_pointer_type).
> > > 
> > > Bootstrapped/regtested x86_64-linux, OK?
> > 
> > Hmm, but make_pointer_type might as well create new pointer types
> > that didn't exist before - shouldn't those have the same problem
> > (which actually is?)?  Note the pointed-to types are already in the IL,
> 
> I am now clearing TYPE_STUB_DECL to NULL and check that it is NULL later in
> streaming.  There are indeed some other cases where this check triggers.
> 
> Pointer types are not problem because they are created w/o stubs.  Other 
> issues
> I see with this are backend produced structures - gcov, asan and ubsan all
> create structures with non-NULL stubs.  They are DECL_ARTIFICIAL so I am not
> quite sure why we set stubs there.
> 
>   tree type_decl = build_decl (input_location, TYPE_DECL, 
>   
>get_identifier ("__asan_global"), ret);
>   
>   DECL_IGNORED_P (type_decl) = 1; 
>   
>   DECL_ARTIFICIAL (type_decl) = 1;
>   
>   TYPE_FIELDS (ret) = fields[0];  
>   
>   TYPE_NAME (ret) = type_decl;
>   
>   TYPE_STUB_DECL (ret) = type_decl;   
>   
> 
> It seems to me that setting TYPE_NAME to identifier node would be easier.

Yeah.  I probably added the DECL_INGORED_P to most of those so indeed
type_decls are somewhat pointless.

> gcov uses finish_builtin_struct which in turn does:
> 
> #if 0 /* not yet, should get fixed properly later */  
>   
>   TYPE_NAME (type) = make_type_decl (get_identifier (name), type);
> #else
>   TYPE_NAME (type) = build_decl (BUILTINS_LOCATION,
>  TYPE_DECL, get_identifier (name), type);
> #endif
>   TYPE_STUB_DECL (type) = TYPE_NAME (type);
> 
> It does not seem to set artificial.  I am not quite sure what to do about
> those.

given gcov has a runtime component with debug info did we intend to
make the compiler-generated part debuggable maybe?

> In any case we should free lang data frontend created pointer types that may
> get back to IL stream.  C++ will give them TYPE_DECL based TYPE_NAMEs that
> we want to translate to identifiers for example.

Ah, yes, that makes sense.  Which means your patch is OK if you reflect
the above in the comment (it's about FE generated pointer types).  We
might also somehow forcefully "collect" unused pointer types from the
chains...

Richard.

> Honza
> 
> 

-- 
Richard Biener 
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 
21284 (AG Nuernberg)

Re: Walk pointer_to and reference_to chain in free_lang_data

[Jan Hubicka]

> >   tree type_decl = build_decl (input_location, TYPE_DECL,   
> > 
> >get_identifier ("__asan_global"), ret);  
> > 
> >   DECL_IGNORED_P (type_decl) = 1;   
> > 
> >   DECL_ARTIFICIAL (type_decl) = 1;  
> > 
> >   TYPE_FIELDS (ret) = fields[0];
> > 
> >   TYPE_NAME (ret) = type_decl;  
> > 
> >   TYPE_STUB_DECL (ret) = type_decl; 
> > 
> > 
> > It seems to me that setting TYPE_NAME to identifier node would be easier.
> 
> Yeah.  I probably added the DECL_INGORED_P to most of those so indeed
> type_decls are somewhat pointless.

OK, I will send separate patch and turn those into identifier nodes.
> 
> > gcov uses finish_builtin_struct which in turn does:
> > 
> > #if 0 /* not yet, should get fixed properly later */
> > 
> >   TYPE_NAME (type) = make_type_decl (get_identifier (name), type);
> > #else
> >   TYPE_NAME (type) = build_decl (BUILTINS_LOCATION,
> >  TYPE_DECL, get_identifier (name), type);
> > #endif
> >   TYPE_STUB_DECL (type) = TYPE_NAME (type);
> > 
> > It does not seem to set artificial.  I am not quite sure what to do about
> > those.
> 
> given gcov has a runtime component with debug info did we intend to
> make the compiler-generated part debuggable maybe?

It never appeared to me that I could debug them and source level :).
Compiling simple main function with profile gnerate we get:
__gcov_.main
__gcov0.main
__gcov7.main
(counters and the structure describing them)
Neither of those seems to be accessible from gdb nor we generate debug info
for the structures we finalize builtins for.

Note that with this patch and sanity check about type_stub_decl being NULL I 
get the following
errors and the lto-bootstrap passes (which is not too bad I would say):

 
 Running target unix
+FAIL: gcc.c-torture/compile/pr44686.c   -O2 -flto -fno-use-linker-plugin 
-flto-partition=none  (internal compiler error)
+FAIL: gcc.c-torture/compile/pr44686.c   -O2 -flto -fno-use-linker-plugin 
-flto-partition=none  (test for excess errors)
+FAIL: gcc.c-torture/compile/pr44686.c   -O2 -flto -fuse-linker-plugin 
-fno-fat-lto-objects  (internal compiler error)
+FAIL: gcc.c-torture/compile/pr44686.c   -O2 -flto -fuse-linker-plugin 
-fno-fat-lto-objects  (test for excess errors)
 XPASS: gcc.dg/guality/example.c   -O0  execution test
 XPASS: gcc.dg/guality/example.c   -O1  -DPREVENT_OPTIMIZATION  execution test
 XPASS: gcc.dg/guality/example.c  -Og -DPREVENT_OPTIMIZATION  execution test
@@ -136,15 +140,81 @@
 FAIL: gcc.dg/guality/vla-1.c   -O2 -flto -fuse-linker-plugin 
-fno-fat-lto-objects  -DPREVENT_OPTIMIZATION line 17 sizeof (a) == 6
 FAIL: gcc.dg/guality/vla-1.c   -O3 -g  -DPREVENT_OPTIMIZATION  line 17 sizeof 
(a) == 6
 FAIL: gcc.dg/guality/vla-1.c   -Os  -DPREVENT_OPTIMIZATION  line 17 sizeof (a) 
== 6
+FAIL: gcc.dg/lto/20100430-1 c_lto_20100430-1_0.o assemble, -O2 -fprofile-arcs 
-flto -r -nostdlib (internal compiler error)
+UNRESOLVED: gcc.dg/lto/20100430-1 c_lto_20100430-1_0.o-c_lto_20100430-1_0.o 
execute -O2 -fprofile-arcs -flto -r -nostdlib
+UNRESOLVED: gcc.dg/lto/20100430-1 c_lto_20100430-1_0.o-c_lto_20100430-1_0.o 
link -O2 -fprofile-arcs -flto -r -nostdlib
+FAIL: gcc.dg/lto/pr69188 c_lto_pr69188_0.o assemble,  -flto -O0 
-fprofile-generate  (internal compiler error)
+UNRESOLVED: gcc.dg/lto/pr69188 c_lto_pr69188_0.o-c_lto_pr69188_1.o execute  
-flto -O0 -fprofile-generate 
+UNRESOLVED: gcc.dg/lto/pr69188 c_lto_pr69188_0.o-c_lto_pr69188_1.o link  -flto 
-O0 -fprofile-generate 
+FAIL: gcc.dg/lto/pr69188 c_lto_pr69188_1.o assemble,  -flto -O0 
-fprofile-generate  (internal compiler error)
+FAIL: gcc.dg/torture/pr41261.c   -O2 -flto -fno-use-linker-plugin 
-flto-partition=none  (internal compiler error)
+FAIL: gcc.dg/torture/pr41261.c   -O2 -flto -fno-use-linker-plugin 
-flto-partition=none  (test for excess errors)
+FAIL: gcc.dg/torture/pr41261.c   -O2 -flto -fuse-linker-plugin 
-fno-fat-lto-objects  (internal compiler error)
+FAIL: gcc.dg/torture/pr41261.c   -O2 -flto -fuse-linker-plugin 
-fno-fat-lto-objects  (test for excess errors)
+FAIL: gcc.dg/torture/pr83055.c   -O2 -flto -fno-use-linker-plugin 
-flto-partition=none  (internal compiler error)
+FAIL: gcc.dg/torture/pr83055.c   -O2 -flto -fno-use-linker-plugin 
-flto-partition=none  (test for excess errors)
+FAIL: gcc.dg/torture/pr83055.c   -O2 -flto -fuse-linker-plugin 
-fno-fat-lto-objects  (internal compiler error)
+FAIL: gcc.dg/torture/pr83055.c   -O2 -flto -fuse-linker-plugin 
-fno-fat-lto-objects  (test for excess errors)
+FAIL: gcc.dg/tree-prof/crossmodule-indircall-1.c compilation,  
-fprofile-generate -D_PROFILE_GENERATE (internal compiler error)
+UNRESOLVED: gcc.dg/tree-prof/crossmodule-indircall-1.c

[PATCH] Fix PR86945

[Richard Biener]

The following fixes PR86945, single-case switch conversion to
range check using signed arithmetic that can overflow.

Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.

Richard.

2018-08-22  Richard Biener  

PR tree-optimization/86945
* tree-cfg.c (generate_range_test): Use unsigned arithmetic.

* gcc.dg/torture/pr86945.c: New testcase.

Index: gcc/tree-cfg.c
===
--- gcc/tree-cfg.c  (revision 263705)
+++ gcc/tree-cfg.c  (working copy)
@@ -9131,20 +9131,16 @@ generate_range_test (basic_block bb, tre
   tree type = TREE_TYPE (index);
   tree utype = unsigned_type_for (type);
 
-  low = fold_convert (type, low);
-  high = fold_convert (type, high);
+  low = fold_convert (utype, low);
+  high = fold_convert (utype, high);
 
-  tree tmp = make_ssa_name (type);
-  gassign *sub1
-= gimple_build_assign (tmp, MINUS_EXPR, index, low);
+  gimple_seq seq = NULL;
+  index = gimple_convert (&seq, utype, index);
+  *lhs = gimple_build (&seq, MINUS_EXPR, utype, index, low);
+  *rhs = const_binop (MINUS_EXPR, utype, high, low);
 
-  *lhs = make_ssa_name (utype);
-  gassign *a = gimple_build_assign (*lhs, NOP_EXPR, tmp);
-
-  *rhs = fold_build2 (MINUS_EXPR, utype, high, low);
   gimple_stmt_iterator gsi = gsi_last_bb (bb);
-  gsi_insert_before (&gsi, sub1, GSI_SAME_STMT);
-  gsi_insert_before (&gsi, a, GSI_SAME_STMT);
+  gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
 }
 
 /* Emit return warnings.  */
Index: gcc/testsuite/gcc.dg/torture/pr86945.c
===
--- gcc/testsuite/gcc.dg/torture/pr86945.c  (nonexistent)
+++ gcc/testsuite/gcc.dg/torture/pr86945.c  (working copy)
@@ -0,0 +1,18 @@
+/* { dg-do run } */
+
+void __attribute__((noinline,noipa))
+foo(int id)
+{
+  switch (id)
+{
+case (-__INT_MAX__ - 1)...-1:
+  __builtin_abort ();
+default:;
+}
+}
+
+int main()
+{
+  foo(1);
+  return 0;
+}

Re: [PATCH] Add a character size parameter to c_strlen/get_range_strlen

[Bernd Edlinger]

On 08/22/18 09:26, Richard Biener wrote:
> On Wed, 22 Aug 2018, Bernd Edlinger wrote:
> 
>> On 08/21/18 10:59, Richard Biener wrote:
>>> On Tue, 21 Aug 2018, Bernd Edlinger wrote:
>>>
 gcc -S -O2 -Wall -Wformat-overflow -ftrack-macro-expansion=0 -fshort-wchar 
 builtin-sprintf-warn-20.c
 builtin-sprintf-warn-20.c: In function 'test':
 builtin-sprintf-warn-20.c:19:39: warning: hex escape sequence out of range
 19 | ? (char*)L"\x4142\x4344" : (char*)L"\x41424344\x45464748";
   |   ^~~

 Hmm, this test might create some noise on short-wchar targets.

 I would prefer a warning here, about the wrong type of the parameter.
 The buffer overflow is only a secondary thing.

 For constant objects like those, the GIMPLE type is still guaranteed to be 
 reliable,
 right?
>>>
>>> TREE_TYPE of tcc_declaration and tcc_constant trees should more-or-less
>>> (minus qualifications not affecting semantics) be those set by
>>> frontends.
>>>
>>
>> and in this case:
>>
>> const union
>> { struct {
>>   wchar_t x[4];
>> };
>> struct {
>>   char z[8];
>> };
>> } u = {{L"123"}};
>>
>> int test()
>> {
>> return __builtin_strlen(u.z);
>> }
>>
>>
>> string_constant works out the initializer for u.x
>> which has a different type than u.z
> 
> Yes.  That's because it uses ctor-for-folding and friends.  It's
> a question of the desired semantics of string_constant whether
> it should better return NULL_TREE in this case or whether the
> caller has to deal with type mismatches.
> 

Yes, absolutely.

c_getstr needs to bail out if the string is not zero-terminated
within the limits given by the decl, or the string_cst-type or whatever
may help.

Furthermore I also consider it possible that the byteoffset
is not a multiple of eltsize.  So fail in that case as well.


I am currently boot-strapping a patch for this (pr87053):
$ cat u.c
const union
{ struct {
 char x[4];
 char y[4];
   };
   struct {
 char z[8];
   };
} u = {{"1234", "567"}};

int test()
{
   return __builtin_strlen(u.z);
}

gets folded to 4.

... but unfortunately it will depend on my pr86714 fix which fixes
the mem_size parameter returned from string_constant.

Frankly, in the moment I feel like I fell in a deep deep hole.   :-O


Bernd.

Re: [PATCH] Add a character size parameter to c_strlen/get_range_strlen

[Richard Biener]

On Wed, 22 Aug 2018, Bernd Edlinger wrote:

> On 08/22/18 09:26, Richard Biener wrote:
> > On Wed, 22 Aug 2018, Bernd Edlinger wrote:
> > 
> >> On 08/21/18 10:59, Richard Biener wrote:
> >>> On Tue, 21 Aug 2018, Bernd Edlinger wrote:
> >>>
>  gcc -S -O2 -Wall -Wformat-overflow -ftrack-macro-expansion=0 
>  -fshort-wchar builtin-sprintf-warn-20.c
>  builtin-sprintf-warn-20.c: In function 'test':
>  builtin-sprintf-warn-20.c:19:39: warning: hex escape sequence out of 
>  range
>  19 | ? (char*)L"\x4142\x4344" : (char*)L"\x41424344\x45464748";
>    |   ^~~
> 
>  Hmm, this test might create some noise on short-wchar targets.
> 
>  I would prefer a warning here, about the wrong type of the parameter.
>  The buffer overflow is only a secondary thing.
> 
>  For constant objects like those, the GIMPLE type is still guaranteed to 
>  be reliable,
>  right?
> >>>
> >>> TREE_TYPE of tcc_declaration and tcc_constant trees should more-or-less
> >>> (minus qualifications not affecting semantics) be those set by
> >>> frontends.
> >>>
> >>
> >> and in this case:
> >>
> >> const union
> >> { struct {
> >>   wchar_t x[4];
> >> };
> >> struct {
> >>   char z[8];
> >> };
> >> } u = {{L"123"}};
> >>
> >> int test()
> >> {
> >> return __builtin_strlen(u.z);
> >> }
> >>
> >>
> >> string_constant works out the initializer for u.x
> >> which has a different type than u.z
> > 
> > Yes.  That's because it uses ctor-for-folding and friends.  It's
> > a question of the desired semantics of string_constant whether
> > it should better return NULL_TREE in this case or whether the
> > caller has to deal with type mismatches.
> > 
> 
> Yes, absolutely.
> 
> c_getstr needs to bail out if the string is not zero-terminated
> within the limits given by the decl, or the string_cst-type or whatever
> may help.
> 
> Furthermore I also consider it possible that the byteoffset
> is not a multiple of eltsize.  So fail in that case as well.
> 
> 
> I am currently boot-strapping a patch for this (pr87053):
> $ cat u.c
> const union
> { struct {
>  char x[4];
>  char y[4];
>};
>struct {
>  char z[8];
>};
> } u = {{"1234", "567"}};
> 
> int test()
> {
>return __builtin_strlen(u.z);
> }
> 
> gets folded to 4.
> 
> ... but unfortunately it will depend on my pr86714 fix which fixes
> the mem_size parameter returned from string_constant.
> 
> Frankly, in the moment I feel like I fell in a deep deep hole.   :-O

/me too with > 100 mails in this and related threads unread ;)

I thought Jeff applied the mem_size parameter thing but maybe it
was something else.  *fetches coffee*  Ah, Jeff is still "working"
on it.

Richard.

Only test STMT_VINFO_STRIDED_P for the first statement in a group

[Richard Sandiford]

get_load_store_type & co were testing STMT_VINFO_STRIDED_P on individual
statements in a group instead of the first.  This has no effect on
its own, but is needed by a later patch.

Tested on aarch64-linux-gnu (with and without SVE), aarch64_be-elf
and x86_64-linux-gnu.  OK to install?

Richard


2018-08-22  Richard Sandiford  

gcc/
* tree-vect-stmts.c (get_group_load_store_type)
(get_load_store_type): Only test STMT_VINFO_STRIDED_P for the
first statement in a group.

Index: gcc/tree-vect-stmts.c
===
--- gcc/tree-vect-stmts.c   2018-08-09 15:37:12.802984133 +0100
+++ gcc/tree-vect-stmts.c   2018-08-22 10:23:50.562747188 +0100
@@ -2191,14 +2191,14 @@ get_group_load_store_type (stmt_vec_info
 
   /* There can only be a gap at the end of the group if the stride is
  known at compile time.  */
-  gcc_assert (!STMT_VINFO_STRIDED_P (stmt_info) || gap == 0);
+  gcc_assert (!STMT_VINFO_STRIDED_P (first_stmt_info) || gap == 0);
 
   /* Stores can't yet have gaps.  */
   gcc_assert (slp || vls_type == VLS_LOAD || gap == 0);
 
   if (slp)
 {
-  if (STMT_VINFO_STRIDED_P (stmt_info))
+  if (STMT_VINFO_STRIDED_P (first_stmt_info))
{
  /* Try to use consecutive accesses of DR_GROUP_SIZE elements,
 separated by the stride, until we have a complete vector.
@@ -2255,7 +2255,7 @@ get_group_load_store_type (stmt_vec_info
/ vect_get_scalar_dr_size (first_dr_info)))
would_overrun_p = false;
 
-  if (!STMT_VINFO_STRIDED_P (stmt_info)
+  if (!STMT_VINFO_STRIDED_P (first_stmt_info)
  && (can_overrun_p || !would_overrun_p)
  && compare_step_with_zero (stmt_info) > 0)
{
@@ -2466,8 +2466,11 @@ get_load_store_type (stmt_vec_info stmt_
   /* FIXME: At the moment the cost model seems to underestimate the
  cost of using elementwise accesses.  This check preserves the
  traditional behavior until that can be fixed.  */
+  stmt_vec_info first_stmt_info = DR_GROUP_FIRST_ELEMENT (stmt_info);
+  if (!first_stmt_info)
+first_stmt_info = stmt_info;
   if (*memory_access_type == VMAT_ELEMENTWISE
-  && !STMT_VINFO_STRIDED_P (stmt_info)
+  && !STMT_VINFO_STRIDED_P (first_stmt_info)
   && !(stmt_info == DR_GROUP_FIRST_ELEMENT (stmt_info)
   && !DR_GROUP_NEXT_ELEMENT (stmt_info)
   && !pow2p_hwi (DR_GROUP_SIZE (stmt_info

[1/2] Fix bogus double reduction (PR 86725)

[Richard Sandiford]

This patch is the first part of the fix for PR 86725.  We would
treat x_1 in:

outer1:
  x_1 = PHI ;
  ...

inner:
  x_2 = ...x_1...;
  ...
  x_3 = ...;
  ...

outer2:
  x_4 = PHI ;
  ...

as a double reduction without checking what kind of statement x_2 is.
In practice it has to be a phi, since for other x_2, x_1 would simply
be a loop invariant that gets used for every inner loop iteration.

The idea with doing this patch first is that, by checking x_2 really
is a phi, we can hand off the validation of the rest of the reduction
to the phi analysis in the inner loop.

The test case is a variant of the one in the PR.

Tested on aarch64-linux-gnu (with and without SVE), aarch64_be-elf
and x86_64-linux-gnu.  OK for trunk and GCC 8?

Richard


2018-08-22  Richard Sandiford  

gcc/
PR tree-optimization/86725
* tree-vect-loop.c (vect_is_simple_reduction): When treating
an outer loop phi as a double reduction, make sure that the
single user of the phi result is an inner loop phi.

gcc/testsuite/
PR tree-optimization/86725
* gcc.dg/vect/no-scevccp-pr86725-1.c: New test.

Index: gcc/tree-vect-loop.c
===
--- gcc/tree-vect-loop.c2018-08-21 14:47:07.047174151 +0100
+++ gcc/tree-vect-loop.c2018-08-22 10:25:06.682099699 +0100
@@ -2992,6 +2992,7 @@ vect_is_simple_reduction (loop_vec_info
   && loop->inner
   && flow_bb_inside_loop_p (loop->inner, gimple_bb (def1))
   && is_gimple_assign (def1)
+ && is_a  (phi_use_stmt)
  && flow_bb_inside_loop_p (loop->inner, gimple_bb (phi_use_stmt)))
 {
   if (dump_enabled_p ())
Index: gcc/testsuite/gcc.dg/vect/no-scevccp-pr86725-1.c
===
--- /dev/null   2018-07-26 10:26:13.137955424 +0100
+++ gcc/testsuite/gcc.dg/vect/no-scevccp-pr86725-1.c2018-08-22 
10:25:06.682099699 +0100
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-O -w" } */
+
+int foo;
+int
+nr (int xe, int z)
+{
+  int oo, wo = 0;
+
+  for (oo = 0; oo < 4; ++oo)
+{
+  int qq;
+
+  int old_wo = wo;
+  for (qq = 0; qq < 2; ++qq)
+{
+  wo = z + qq + old_wo;
+  xe += wo;
+}
+}
+  foo = wo;
+  return xe;
+}
+
+/* { dg-final { scan-tree-dump-not "double reduction: wo" "vect" } } */
+/* { dg-final { scan-tree-dump-not "OUTER LOOP VECTORIZED" "vect" } } */

VRP: make range_includes_zero_p handle value_ranges

[Aldy Hernandez]

On 08/21/2018 05:46 AM, Richard Biener wrote:

On Wed, Aug 15, 2018 at 3:33 AM Aldy Hernandez  wrote:



Finally, my apologies for including a tiny change to the
POINTER_PLUS_EXPR handling code as well.  It came about the same set of
auditing tests.


Bah, please split up things here ;)  I've done a related change there
yesterday...



It turns out we can handle POINTER_PLUS_EXPR(~[0,0], [X,Y]) without
bailing as VR_VARYING in extract_range_from_binary_expr_1.  In doing so,
I also noticed that ~[0,0] is not the only non-null.  We could also have
~[0,2] and still know that the pointer is not zero.  I have adjusted
range_is_nonnull accordingly.


But there are other consumers and it would have been better to
change range_includes_zero_p to do the natural thing (get a VR) and
then remove range_is_nonnull as redundant if possible.


Indeed.  Cleaning up range_includes_zero_p makes VRP and friends a lot 
cleaner.  Thanks for the suggestion.


I lazily avoided cleaning up the division code affected in this patch 
too much, since it's going to be superseded by my division changes in 
the other patch.


OK pending tests?

Aldy
gcc/

	* gimple-ssa-evrp-analyze.c (set_ssa_range_info): Pass value_range
	to range_includes_zero_p.  Do not special case VR_ANTI_RANGE.
	* tree-vrp.c (range_is_nonnull): Remove.
	(range_includes_zero_p): Accept value_range instead of min/max.
	(extract_range_from_binary_expr_1): Do not early bail on
	POINTER_PLUS_EXPR.
	Use range_includes_zero_p instead of range_is_nonnull.
	(extract_range_from_unary_expr): Use range_includes_zero_p instead
	of range_is_nonnull.
	(vrp_meet_1): Pass value_range to range_includes_zero_p.  Do not
	special case VR_ANTI_RANGE.
	(vrp_finalize): Same.
	* tree-vrp.h (range_includes_zero_p): Pass value_range as argument
	instead of min/max.
	(range_is_nonnull): Remove.
	* vr-values.c (vrp_stmt_computes_nonzero): Use
	range_includes_zero_p instead of range_is_nonnull.
	(extract_range_basic): Pass value_range to range_includes_zero_p
	instead of range_is_nonnull.

diff --git a/gcc/gimple-ssa-evrp-analyze.c b/gcc/gimple-ssa-evrp-analyze.c
index b9dcf906ff7..e9afa80e191 100644
--- a/gcc/gimple-ssa-evrp-analyze.c
+++ b/gcc/gimple-ssa-evrp-analyze.c
@@ -119,12 +119,7 @@ evrp_range_analyzer::set_ssa_range_info (tree lhs, value_range *vr)
 			wi::to_wide (vr->max));
 }
   else if (POINTER_TYPE_P (TREE_TYPE (lhs))
-	   && ((vr->type == VR_RANGE
-		&& range_includes_zero_p (vr->min,
-	  vr->max) == 0)
-	   || (vr->type == VR_ANTI_RANGE
-		   && range_includes_zero_p (vr->min,
-	 vr->max) == 1)))
+	   && range_includes_zero_p (vr) == 0)
 set_ptr_nonnull (lhs);
 }
 
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 24e089b019b..e4f1b0b8da1 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -538,17 +538,6 @@ vrp_bitmap_equal_p (const_bitmap b1, const_bitmap b2)
 	  && bitmap_equal_p (b1, b2)));
 }
 
-/* Return true if VR is ~[0, 0].  */
-
-bool
-range_is_nonnull (value_range *vr)
-{
-  return vr->type == VR_ANTI_RANGE
-	 && integer_zerop (vr->min)
-	 && integer_zerop (vr->max);
-}
-
-
 /* Return true if VR is [0, 0].  */
 
 static inline bool
@@ -916,14 +905,20 @@ value_ranges_intersect_p (value_range *vr0, value_range *vr1)
 }
 
 
-/* Return 1 if [MIN, MAX] includes the value zero, 0 if it does not
-   include the value zero, -2 if we cannot tell.  */
+/* Return 1 if *VR includes the value zero, 0 if it does not include
+   the value zero, or -2 if we cannot tell.  */
 
 int
-range_includes_zero_p (tree min, tree max)
+range_includes_zero_p (const value_range *vr)
 {
-  tree zero = build_int_cst (TREE_TYPE (min), 0);
-  return value_inside_range (zero, min, max);
+  if (vr->type == VR_UNDEFINED || vr->type == VR_VARYING)
+return -2;
+
+  tree zero = build_int_cst (TREE_TYPE (vr->min), 0);
+  if (vr->type == VR_RANGE)
+return value_inside_range (zero, vr->min, vr->max);
+  else
+return !value_inside_range (zero, vr->min, vr->max);
 }
 
 /* Return true if *VR is know to only contain nonnegative values.  */
@@ -1440,7 +1435,10 @@ extract_range_from_binary_expr_1 (value_range *vr,
   && code != POINTER_PLUS_EXPR
   && (vr0.type == VR_VARYING
 	  || vr1.type == VR_VARYING
-	  || vr0.type != vr1.type
+	  || (vr0.type != vr1.type
+	  /* We can handle POINTER_PLUS_EXPR(~[0,0], [x,y]) below,
+		 even though we have differing range kinds.  */
+	  && code != POINTER_PLUS_EXPR)
 	  || symbolic_range_p (&vr0)
 	  || symbolic_range_p (&vr1)))
 {
@@ -1457,7 +1455,7 @@ extract_range_from_binary_expr_1 (value_range *vr,
 	 nullness, if both are non null, then the result is nonnull.
 	 If both are null, then the result is null. Otherwise they
 	 are varying.  */
-	  if (range_is_nonnull (&vr0) && range_is_nonnull (&vr1))
+	  if (!range_includes_zero_p (&vr0) && !range_includes_zero_p (&vr1))
 	set_value_range_to_nonnull (vr, expr_type);
 	  else if (range_is_null (&vr0) && range_is_null (&vr1))
 	set_value_range

[2/2] Fix bogus inner induction (PR 86725)

[Richard Sandiford]

This patch is the second part of the fix for PR 86725.  The problem
in the original test is that for:

  outer1:
x_1 = PHI ;
...

  inner:
x_2 = PHI ;
...
x_3 = ...;
...

  outer2:
x_4 = PHI ;
...

there are corner cases in which it is possible to classify the
inner phi as an induction but not the outer phi.  The -4.c test
is a more direct example.

After failing to classify x_1 as an induction, we go on to
classify it as a double reduction (which is basically true).
But we still classified the inner phi as an induction rather
than as part of a reduction, leading to an ICE when trying
to vectorise the outer phi.

We analyse the phis for outer loops first, so the simplest
fix is not to classify the phi as an induction if outer loop
analysis said that it should be a reduction.

The -2.c test is from the original PR.  The -3.c test is a
version in which "wo" really is used a reduction; this was
already correctly rejected, but for the wrong reason ("inner-loop
induction only used outside of the outer vectorized loop").
The -4.c test is another way of tickling the original problem
without relying on the undefinedness of signed overflow.
The -5.c test shows an (uninteresting) example in which the
patch prevents a spurious failure to vectorise the outer loop.

Tested on aarch64-linux-gnu (with and without SVE), aarch64_be-elf
and x86_64-linux-gnu.  OK for trunk and GCC 8?

Richard


2018-08-22  Richard Sandiford  

gcc/
PR tree-optimization/86725
* tree-vect-loop.c (vect_inner_phi_in_double_reduction_p): New
function.
(vect_analyze_scalar_cycles_1): Check it.

gcc/testsuite/
PR tree-optimization/86725
* gcc.dg/vect/no-scevccp-pr86725-2.c: New test.
* gcc.dg/vect/no-scevccp-pr86725-3.c: Likewise.
* gcc.dg/vect/no-scevccp-pr86725-4.c: Likewise.
* gcc.dg/vect/no-scevccp-pr86725-5.c: Likewise.

Index: gcc/tree-vect-loop.c
===
--- gcc/tree-vect-loop.c2018-08-22 10:25:06.682099699 +0100
+++ gcc/tree-vect-loop.c2018-08-22 10:25:09.586074995 +0100
@@ -462,6 +462,40 @@ vect_is_simple_iv_evolution (unsigned lo
   return true;
 }
 
+/* Return true if PHI, described by STMT_INFO, is the inner PHI in
+   what we are assuming is a double reduction.  For example, given
+   a structure like this:
+
+  outer1:
+   x_1 = PHI ;
+   ...
+
+  inner:
+   x_2 = PHI ;
+   ...
+   x_3 = ...;
+   ...
+
+  outer2:
+   x_4 = PHI ;
+   ...
+
+   outer loop analysis would treat x_1 as a double reduction phi and
+   this function would then return true for x_2.  */
+
+static bool
+vect_inner_phi_in_double_reduction_p (stmt_vec_info stmt_info, gphi *phi)
+{
+  loop_vec_info loop_vinfo = STMT_VINFO_LOOP_VINFO (stmt_info);
+  use_operand_p use_p;
+  ssa_op_iter op_iter;
+  FOR_EACH_PHI_ARG (use_p, phi, op_iter, SSA_OP_USE)
+if (stmt_vec_info def_info = loop_vinfo->lookup_def (USE_FROM_PTR (use_p)))
+  if (STMT_VINFO_DEF_TYPE (def_info) == vect_double_reduction_def)
+   return true;
+  return false;
+}
+
 /* Function vect_analyze_scalar_cycles_1.
 
Examine the cross iteration def-use cycles of scalar variables
@@ -522,6 +556,7 @@ vect_analyze_scalar_cycles_1 (loop_vec_i
}
 
   if (!access_fn
+ || vect_inner_phi_in_double_reduction_p (stmt_vinfo, phi)
  || !vect_is_simple_iv_evolution (loop->num, access_fn, &init, &step)
  || (LOOP_VINFO_LOOP (loop_vinfo) != loop
  && TREE_CODE (step) != INTEGER_CST))
Index: gcc/testsuite/gcc.dg/vect/no-scevccp-pr86725-2.c
===
--- /dev/null   2018-07-26 10:26:13.137955424 +0100
+++ gcc/testsuite/gcc.dg/vect/no-scevccp-pr86725-2.c2018-08-22 
10:25:09.582075029 +0100
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-O -w" } */
+
+int
+nr (int xe)
+{
+  int oo, wo = 0;
+
+  for (oo = 0; oo < 4; ++oo)
+{
+  int qq;
+
+  for (qq = 0; qq < 2; ++qq)
+{
+  wo += 0x8000;
+  xe += wo;
+}
+}
+  return xe;
+}
+
+/* { dg-final { scan-tree-dump "reduction used in loop" "vect" { target 
vect_int } } } */
+/* { dg-final { scan-tree-dump-not "OUTER LOOP VECTORIZED" "vect" } } */
Index: gcc/testsuite/gcc.dg/vect/no-scevccp-pr86725-3.c
===
--- /dev/null   2018-07-26 10:26:13.137955424 +0100
+++ gcc/testsuite/gcc.dg/vect/no-scevccp-pr86725-3.c2018-08-22 
10:25:09.582075029 +0100
@@ -0,0 +1,25 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-O -w" } */
+
+int foo;
+int
+nr (int xe)
+{
+  int oo, wo = 0;
+
+  for (oo = 0; oo < 4; ++oo)
+{
+  int qq;
+
+  for (qq = 0; qq < 2; ++qq)
+{
+  wo += 0x8000;
+  xe += wo;
+}
+}
+  foo = wo;
+  return xe;
+}
+
+/* { dg-final { scan-tree-dump "r

Re: [PATCH][debug] Add -gforce-named-dies

[Tom de Vries]

On 08/22/2018 08:56 AM, Tom de Vries wrote:
> This is an undocumented developer-only option, because using this option may
> change behaviour of dwarf consumers, f.i., gdb shows the artificial variables:
> ...
> (gdb) info locals
> a = 0x7fffda90 "\005"
> D.4278 = 
> ...

I just found in the dwarf 5 spec the attribute DW_AT_description
(present since version 3):
...
2.20 Entity Descriptions
Some debugging information entries may describe entities in the program
that are artificial, or which otherwise have a “name” that is not a
valid identifier in the programming language.

This attribute provides a means for the producer to indicate the purpose
or usage of the containing debugging infor

Generally, any debugging information entry that has, or may have, a
DW_AT_name attribute, may also have a DW_AT_description attribute whose
value is a null-terminated string providing a description of the entity.

It is expected that a debugger will display these descriptions as part
of displaying other properties of an entity.
...

AFAICT, gdb currently does not explicitly handle this attribute, which I
think means it's ignored.

It seems applicable to use DW_AT_description at least for the artificial
decls.

Perhaps even for all cases that are added by the patch?

I'll rewrite the patch.

Thanks,
- Tom

[PATCH] Fix PR86988

[Richard Biener]

Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.

Richard.

2018-08-22  Richard Biener  

PR tree-optimization/86988
* tree-vrp.c (vrp_prop::check_mem_ref): Bail out on VLAs.

* g++.dg/pr86988.C: New testcase.

diff --git a/gcc/testsuite/g++.dg/pr86988.C b/gcc/testsuite/g++.dg/pr86988.C
new file mode 100644
index 000..62fb0f3220d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr86988.C
@@ -0,0 +1,16 @@
+// { dg-do compile }
+// { dg-options "-O2 -Warray-bounds" }
+
+struct R { int r; };
+void baz (char *, char *, char *, char *);
+
+void
+foo ()
+{
+  const R a = { 12 };
+  char b[1][a.r] = { { "12345678901" } };
+  char c[a.r] = { "12345678901" };
+  char d[1][a.r] = { { '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', 
'\0' } };
+  char e[a.r] = { '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '\0' 
};
+  baz (b[0], c, d[0], e);
+}
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 24e089b019b..ead19f15996 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -4581,6 +4581,7 @@ vrp_prop::check_mem_ref (location_t location, tree ref,
   tree reftype = TREE_TYPE (arg);
   if (POINTER_TYPE_P (reftype)
   || !COMPLETE_TYPE_P (reftype)
+  || TREE_CODE (TYPE_SIZE_UNIT (reftype)) != INTEGER_CST
   || RECORD_OR_UNION_TYPE_P (reftype))
 return;
 

Re: patch to bug #86829

[Richard Biener]

On Tue, Aug 21, 2018 at 11:27 PM Jeff Law  wrote:
>
> On 08/21/2018 02:08 PM, Giuliano Augusto Faulin Belinassi wrote:
> >> Just as an example, compare the results for
> >> x = 0x1.fp1023
> >
> > Thank you for your answer and the counterexample. :-)
> >
> >> If we had useful range info on floats we might conditionalize such
> >> transforms appropriately.  Or we can enable it on floats and do
> >> the sqrt (x*x + 1) in double.
> >
> > I think I managed to find a bound were the transformation can be done
> > without overflow harm, however I don't know about rounding problems,
> > however
> >
> > Suppose we are handling double precision floats for now. The function
> > x/sqrt(1 + x*x) approaches 1 when x is big enough. How big must be x
> > for the function be 1?
> >
> > Since sqrt(1 + x*x) > x when x > 1, then we must find a value to x
> > that x/sqrt(1 + x*x) < eps, where eps is the biggest double smaller
> > than 1. Such eps must be around 1 - 2^-53 in ieee double because the
> > mantissa has 52 bits. Solving for x yields that x must be somewhat
> > bigger than 6.7e7, so let's take 1e8. Therefore if abs(x) > 1e8, it is
> > enough to return copysign(1, x). Notice that this arguments is also
> > valid for x = +-inf (if target supports that) because sin(atan(+-inf))
> > = +-1, and it can be extended to other floating point formats.The
> > following test code illustrates my point:
> > https://pastebin.com/M4G4neLQ
> >
> > This might still be faster than calculating sin(atan(x)) explicitly.
> >
> > Please let me know if this is unfeasible. :-)
> The problem is our VRP implementation doesn't handle any floating point
> types at this time.   If we had range information for FP types, then
> this kind of analysis is precisely what we'd need to do the
> transformation regardless of -ffast-math.

I think his idea was to emit a runtime test?  You'd have to use a
COND_EXPR and evaluate both arms at the same time because
match.pd doesn't allow you to create control flow.

Note the rounding issue is also real given for large x you strip
away lower mantissa bits when computing x*x.

Richard.

> jeff
> >
> > Giuliano.
> >
> > On Tue, Aug 21, 2018 at 11:27 AM, Jeff Law  wrote:
> >> On 08/21/2018 02:02 AM, Richard Biener wrote:
> >>> On Mon, Aug 20, 2018 at 9:40 PM Jeff Law  wrote:
> 
>  On 08/04/2018 07:22 AM, Giuliano Augusto Faulin Belinassi wrote:
> > Closes bug #86829
> >
> > Description: Adds substitution rules for both sin(atan(x)) and
> > cos(atan(x)). These formulas are replaced by x / sqrt(x*x + 1) and 1 /
> > sqrt(x*x + 1) respectively, providing up to 10x speedup. This identity
> > can be proved mathematically.
> >
> > Changelog:
> >
> > 2018-08-03  Giuliano Belinassi 
> >
> > * match.pd: add simplification rules to sin(atan(x)) and 
> > cos(atan(x)).
> >
> > Bootstrap and Testing:
> > There were no unexpected failures in a proper testing in GCC 8.1.0
> > under a x86_64 running Ubuntu 18.04.
>  I understand these are mathematical identities.  But floating point
>  arthmetic in a compiler isn't nearly that clean :-)  We have to worry
>  about overflows, underflows, rounding, and the simple fact that many
>  floating point numbers can't be exactly represented.
> 
>  Just as an example, compare the results for
>  x = 0x1.fp1023
> 
>  I think sin(atan (x)) is well defined in that case.  But the x*x isn't
>  because it overflows.
> 
>  So  I think this has to be somewhere under the -ffast-math umbrella.
>  And the testing requirements for that are painful -- you have to verify
>  it doesn't break the spec benchmark.
> 
>  I know Richi acked in the PR, but that might have been premature.
> >>>
> >>> It's under the flag_unsafe_math_optimizations umbrella, but sure,
> >>> a "proper" way to optimize this would be to further expand
> >>> sqrt (x*x + 1) to fabs(x) + ... (extra terms) that are precise enough
> >>> and not have this overflow issue.
> >>>
> >>> But yes, I do not find (quickly skimming) other simplifications that
> >>> have this kind of overflow issue (in fact I do remember raising
> >>> overflow/underflow issues for other patches).
> >>>
> >>> Thus approval withdrawn.
> >> At least until we can do some testing around spec.  There's also a patch
> >> for logarithm addition/subtraction from MCC CS and another from Giuliano
> >> for hyperbolics that need testing with spec.  I think that getting that
> >> testing done anytime between now and stage1 close is sufficient -- none
> >> of the 3 patches is particularly complex.
> >>
> >>
> >>>
> >>> If we had useful range info on floats we might conditionalize such
> >>> transforms appropriately.  Or we can enable it on floats and do
> >>> the sqrt (x*x + 1) in double.
> >> Yea.  I keep thinking about what it might take to start doing some light
> >> VRP of floating point objects.  I'd originally been thi

Re: Only test STMT_VINFO_STRIDED_P for the first statement in a group

[Richard Biener]

On Wed, Aug 22, 2018 at 11:27 AM Richard Sandiford
 wrote:
>
> get_load_store_type & co were testing STMT_VINFO_STRIDED_P on individual
> statements in a group instead of the first.  This has no effect on
> its own, but is needed by a later patch.
>
> Tested on aarch64-linux-gnu (with and without SVE), aarch64_be-elf
> and x86_64-linux-gnu.  OK to install?

OK.

Richard.

> Richard
>
>
> 2018-08-22  Richard Sandiford  
>
> gcc/
> * tree-vect-stmts.c (get_group_load_store_type)
> (get_load_store_type): Only test STMT_VINFO_STRIDED_P for the
> first statement in a group.
>
> Index: gcc/tree-vect-stmts.c
> ===
> --- gcc/tree-vect-stmts.c   2018-08-09 15:37:12.802984133 +0100
> +++ gcc/tree-vect-stmts.c   2018-08-22 10:23:50.562747188 +0100
> @@ -2191,14 +2191,14 @@ get_group_load_store_type (stmt_vec_info
>
>/* There can only be a gap at the end of the group if the stride is
>   known at compile time.  */
> -  gcc_assert (!STMT_VINFO_STRIDED_P (stmt_info) || gap == 0);
> +  gcc_assert (!STMT_VINFO_STRIDED_P (first_stmt_info) || gap == 0);
>
>/* Stores can't yet have gaps.  */
>gcc_assert (slp || vls_type == VLS_LOAD || gap == 0);
>
>if (slp)
>  {
> -  if (STMT_VINFO_STRIDED_P (stmt_info))
> +  if (STMT_VINFO_STRIDED_P (first_stmt_info))
> {
>   /* Try to use consecutive accesses of DR_GROUP_SIZE elements,
>  separated by the stride, until we have a complete vector.
> @@ -2255,7 +2255,7 @@ get_group_load_store_type (stmt_vec_info
> / vect_get_scalar_dr_size (first_dr_info)))
> would_overrun_p = false;
>
> -  if (!STMT_VINFO_STRIDED_P (stmt_info)
> +  if (!STMT_VINFO_STRIDED_P (first_stmt_info)
>   && (can_overrun_p || !would_overrun_p)
>   && compare_step_with_zero (stmt_info) > 0)
> {
> @@ -2466,8 +2466,11 @@ get_load_store_type (stmt_vec_info stmt_
>/* FIXME: At the moment the cost model seems to underestimate the
>   cost of using elementwise accesses.  This check preserves the
>   traditional behavior until that can be fixed.  */
> +  stmt_vec_info first_stmt_info = DR_GROUP_FIRST_ELEMENT (stmt_info);
> +  if (!first_stmt_info)
> +first_stmt_info = stmt_info;
>if (*memory_access_type == VMAT_ELEMENTWISE
> -  && !STMT_VINFO_STRIDED_P (stmt_info)
> +  && !STMT_VINFO_STRIDED_P (first_stmt_info)
>&& !(stmt_info == DR_GROUP_FIRST_ELEMENT (stmt_info)
>&& !DR_GROUP_NEXT_ELEMENT (stmt_info)
>&& !pow2p_hwi (DR_GROUP_SIZE (stmt_info

Re: [1/2] Fix bogus double reduction (PR 86725)

[Richard Biener]

On Wed, Aug 22, 2018 at 11:31 AM Richard Sandiford
 wrote:
>
> This patch is the first part of the fix for PR 86725.  We would
> treat x_1 in:
>
> outer1:
>   x_1 = PHI ;
>   ...
>
> inner:
>   x_2 = ...x_1...;
>   ...
>   x_3 = ...;
>   ...
>
> outer2:
>   x_4 = PHI ;
>   ...
>
> as a double reduction without checking what kind of statement x_2 is.
> In practice it has to be a phi, since for other x_2, x_1 would simply
> be a loop invariant that gets used for every inner loop iteration.
>
> The idea with doing this patch first is that, by checking x_2 really
> is a phi, we can hand off the validation of the rest of the reduction
> to the phi analysis in the inner loop.
>
> The test case is a variant of the one in the PR.
>
> Tested on aarch64-linux-gnu (with and without SVE), aarch64_be-elf
> and x86_64-linux-gnu.  OK for trunk and GCC 8?

OK.

Thanks,
Richard.

> Richard
>
>
> 2018-08-22  Richard Sandiford  
>
> gcc/
> PR tree-optimization/86725
> * tree-vect-loop.c (vect_is_simple_reduction): When treating
> an outer loop phi as a double reduction, make sure that the
> single user of the phi result is an inner loop phi.
>
> gcc/testsuite/
> PR tree-optimization/86725
> * gcc.dg/vect/no-scevccp-pr86725-1.c: New test.
>
> Index: gcc/tree-vect-loop.c
> ===
> --- gcc/tree-vect-loop.c2018-08-21 14:47:07.047174151 +0100
> +++ gcc/tree-vect-loop.c2018-08-22 10:25:06.682099699 +0100
> @@ -2992,6 +2992,7 @@ vect_is_simple_reduction (loop_vec_info
>&& loop->inner
>&& flow_bb_inside_loop_p (loop->inner, gimple_bb (def1))
>&& is_gimple_assign (def1)
> + && is_a  (phi_use_stmt)
>   && flow_bb_inside_loop_p (loop->inner, gimple_bb (phi_use_stmt)))
>  {
>if (dump_enabled_p ())
> Index: gcc/testsuite/gcc.dg/vect/no-scevccp-pr86725-1.c
> ===
> --- /dev/null   2018-07-26 10:26:13.137955424 +0100
> +++ gcc/testsuite/gcc.dg/vect/no-scevccp-pr86725-1.c2018-08-22 
> 10:25:06.682099699 +0100
> @@ -0,0 +1,26 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-O -w" } */
> +
> +int foo;
> +int
> +nr (int xe, int z)
> +{
> +  int oo, wo = 0;
> +
> +  for (oo = 0; oo < 4; ++oo)
> +{
> +  int qq;
> +
> +  int old_wo = wo;
> +  for (qq = 0; qq < 2; ++qq)
> +{
> +  wo = z + qq + old_wo;
> +  xe += wo;
> +}
> +}
> +  foo = wo;
> +  return xe;
> +}
> +
> +/* { dg-final { scan-tree-dump-not "double reduction: wo" "vect" } } */
> +/* { dg-final { scan-tree-dump-not "OUTER LOOP VECTORIZED" "vect" } } */

Re: [2/2] Fix bogus inner induction (PR 86725)

[Richard Biener]

On Wed, Aug 22, 2018 at 11:34 AM Richard Sandiford
 wrote:
>
> This patch is the second part of the fix for PR 86725.  The problem
> in the original test is that for:
>
>   outer1:
> x_1 = PHI ;
> ...
>
>   inner:
> x_2 = PHI ;
> ...
> x_3 = ...;
> ...
>
>   outer2:
> x_4 = PHI ;
> ...
>
> there are corner cases in which it is possible to classify the
> inner phi as an induction but not the outer phi.  The -4.c test
> is a more direct example.
>
> After failing to classify x_1 as an induction, we go on to
> classify it as a double reduction (which is basically true).
> But we still classified the inner phi as an induction rather
> than as part of a reduction, leading to an ICE when trying
> to vectorise the outer phi.
>
> We analyse the phis for outer loops first, so the simplest
> fix is not to classify the phi as an induction if outer loop
> analysis said that it should be a reduction.
>
> The -2.c test is from the original PR.  The -3.c test is a
> version in which "wo" really is used a reduction; this was
> already correctly rejected, but for the wrong reason ("inner-loop
> induction only used outside of the outer vectorized loop").
> The -4.c test is another way of tickling the original problem
> without relying on the undefinedness of signed overflow.
> The -5.c test shows an (uninteresting) example in which the
> patch prevents a spurious failure to vectorise the outer loop.
>
> Tested on aarch64-linux-gnu (with and without SVE), aarch64_be-elf
> and x86_64-linux-gnu.  OK for trunk and GCC 8?

OK.

Thanks,
Richard.

> Richard
>
>
> 2018-08-22  Richard Sandiford  
>
> gcc/
> PR tree-optimization/86725
> * tree-vect-loop.c (vect_inner_phi_in_double_reduction_p): New
> function.
> (vect_analyze_scalar_cycles_1): Check it.
>
> gcc/testsuite/
> PR tree-optimization/86725
> * gcc.dg/vect/no-scevccp-pr86725-2.c: New test.
> * gcc.dg/vect/no-scevccp-pr86725-3.c: Likewise.
> * gcc.dg/vect/no-scevccp-pr86725-4.c: Likewise.
> * gcc.dg/vect/no-scevccp-pr86725-5.c: Likewise.
>
> Index: gcc/tree-vect-loop.c
> ===
> --- gcc/tree-vect-loop.c2018-08-22 10:25:06.682099699 +0100
> +++ gcc/tree-vect-loop.c2018-08-22 10:25:09.586074995 +0100
> @@ -462,6 +462,40 @@ vect_is_simple_iv_evolution (unsigned lo
>return true;
>  }
>
> +/* Return true if PHI, described by STMT_INFO, is the inner PHI in
> +   what we are assuming is a double reduction.  For example, given
> +   a structure like this:
> +
> +  outer1:
> +   x_1 = PHI ;
> +   ...
> +
> +  inner:
> +   x_2 = PHI ;
> +   ...
> +   x_3 = ...;
> +   ...
> +
> +  outer2:
> +   x_4 = PHI ;
> +   ...
> +
> +   outer loop analysis would treat x_1 as a double reduction phi and
> +   this function would then return true for x_2.  */
> +
> +static bool
> +vect_inner_phi_in_double_reduction_p (stmt_vec_info stmt_info, gphi *phi)
> +{
> +  loop_vec_info loop_vinfo = STMT_VINFO_LOOP_VINFO (stmt_info);
> +  use_operand_p use_p;
> +  ssa_op_iter op_iter;
> +  FOR_EACH_PHI_ARG (use_p, phi, op_iter, SSA_OP_USE)
> +if (stmt_vec_info def_info = loop_vinfo->lookup_def (USE_FROM_PTR 
> (use_p)))
> +  if (STMT_VINFO_DEF_TYPE (def_info) == vect_double_reduction_def)
> +   return true;
> +  return false;
> +}
> +
>  /* Function vect_analyze_scalar_cycles_1.
>
> Examine the cross iteration def-use cycles of scalar variables
> @@ -522,6 +556,7 @@ vect_analyze_scalar_cycles_1 (loop_vec_i
> }
>
>if (!access_fn
> + || vect_inner_phi_in_double_reduction_p (stmt_vinfo, phi)
>   || !vect_is_simple_iv_evolution (loop->num, access_fn, &init, &step)
>   || (LOOP_VINFO_LOOP (loop_vinfo) != loop
>   && TREE_CODE (step) != INTEGER_CST))
> Index: gcc/testsuite/gcc.dg/vect/no-scevccp-pr86725-2.c
> ===
> --- /dev/null   2018-07-26 10:26:13.137955424 +0100
> +++ gcc/testsuite/gcc.dg/vect/no-scevccp-pr86725-2.c2018-08-22 
> 10:25:09.582075029 +0100
> @@ -0,0 +1,23 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-O -w" } */
> +
> +int
> +nr (int xe)
> +{
> +  int oo, wo = 0;
> +
> +  for (oo = 0; oo < 4; ++oo)
> +{
> +  int qq;
> +
> +  for (qq = 0; qq < 2; ++qq)
> +{
> +  wo += 0x8000;
> +  xe += wo;
> +}
> +}
> +  return xe;
> +}
> +
> +/* { dg-final { scan-tree-dump "reduction used in loop" "vect" { target 
> vect_int } } } */
> +/* { dg-final { scan-tree-dump-not "OUTER LOOP VECTORIZED" "vect" } } */
> Index: gcc/testsuite/gcc.dg/vect/no-scevccp-pr86725-3.c
> ===
> --- /dev/null   2018-07-26 10:26:13.137955424 +0100
> +++ gcc/testsuite/gcc.dg/vect/no-scevccp-pr86725-3.c2018-08-22 
> 10:25:09.582075029 +0100
> @@ -

[PATCH] PR libstdc++/77854 document size_type for containers

[Jonathan Wakely]

PR libstdc++/77854
* doc/xml/manual/status_cxx1998.xml: Document size_type and
difference_type for containers.
* doc/html/*: Regenerate.

Committed to trunk.


commit 6477192f0b0c0c482f3d8e5ada1bcada0d200ddf
Author: Jonathan Wakely 
Date:   Wed Aug 22 13:08:56 2018 +0100

PR libstdc++/77854 document size_type for containers

PR libstdc++/77854
* doc/xml/manual/status_cxx1998.xml: Document size_type and
difference_type for containers.
* doc/html/*: Regenerate.

diff --git a/libstdc++-v3/doc/xml/manual/status_cxx1998.xml 
b/libstdc++-v3/doc/xml/manual/status_cxx1998.xml
index 6afb016a45f..2b05ff6601a 100644
--- a/libstdc++-v3/doc/xml/manual/status_cxx1998.xml
+++ b/libstdc++-v3/doc/xml/manual/status_cxx1998.xml
@@ -1126,6 +1126,10 @@ particular release.
   implementation will be described under
   Localization.

+   [23.*] All of the containers in this clause
+ define size_type as std::size_t and
+ difference_type as std::ptrdiff_t.
+   
[26.2.8]/9 I have no idea what
   complex's pow(0,0) returns.

Re: [PATCH][debug] Fix handling of vlas in lto

[Richard Biener]

On Tue, 21 Aug 2018, Tom de Vries wrote:

> On 08/20/2018 03:09 PM, Richard Biener wrote:
> > On Fri, 17 Aug 2018, Tom de Vries wrote:
> > 
> >> I've rewritten the patch to work generically, not just for 
> >> DW_AT_upper_bound,
> >> and to reuse the code already there in add_scalar_info.
> >>
> >> OK for trunk?
> >>
> >> Thanks,
> >> - Tom
> >>
> >> [debug] Fix handling of vlas in lto
> >>
> >> Atm, when running vla-1.c with -O0 -flto, we have:
> >> ...
> >> FAIL: gcc.dg/guality/vla-1.c -O0 -flto -fuse-linker-plugin \
> >>   -fno-fat-lto-objects line 17 sizeof (a) == 6
> >> ...
> >>
> >> The vla a[i + 1] in f1 is gimplified into:
> >> ...
> >> f1 (int i)
> >> {
> >>   char a[0:D.1922] [value-expr: *a.0];
> >>   char[0:D.1922] * a.0;
> >>
> >>   D.1921 = i + 1;
> >>   D.1926 = (sizetype) D.1921;
> >>   a.0 = __builtin_alloca_with_align (D.1926, 8);
> >> ...
> >>
> >> The early debug info for the upper bound of the type of vla a that we 
> >> stream
> >> out is:
> >> ...
> >>   DIE0: DW_TAG_subrange_type (0x7f85029a90f0)
> >> DW_AT_upper_bound: location descriptor:
> >>   (0x7f85029a9230) DW_OP_GNU_variable_value die -> 0 (0x7f85029a94b0), > >> 0
> >>   DIE0: DW_TAG_variable (0x7f85029a94b0)
> >> DW_AT_name: "D.1922"
> >> DW_AT_type: die -> 0 (0x7f85029a3d70)
> >> DW_AT_artificial: 1
> >> ...
> >>
> >> and in ltrans we have for that same upper bound:
> >> ...
> >>   DIE0: DW_TAG_subrange_type (0x7f5183b57d70)
> >> DW_AT_upper_bound: die -> 0 (0x7f5183b576e0)
> >>   DIE0: DW_TAG_variable (0x7f5183b576e0)
> >> DW_AT_name: "D.4278"
> >> DW_AT_abstract_origin: die -> label: vla_1.c.6719312a + 193 
> >> (0x7f5183b57730)
> >> ...
> >> where D.4278 has abstract origin D.1922.
> >>
> >> The D.4278 die has no DW_AT_location, so when evaluting "sizeof (a)" in the
> >> debugger, we can't find the information to get the value of D.4278, and the
> >> debugger prints "".
> >>
> >> This patch fixes that by either:
> >> - adding DW_AT_location to the referenced variable die, or
> >> - instead of using a ref for the upper bound, using an exprloc.
> >>
> >> When changing gcc.dg/guality/guality.exp to run the usual flto flavours
> >> "-fno-use-linker-plugin -flto-partition=none" and "-fuse-linker-plugin
> >> -fno-fat-lto-objects" in combination with O0, Og, O1, O2, O3 and Os, this 
> >> patch
> >> fixes all (20) failures in vla-1.c, leaving only:
> >> ...
> >> No symbol "i" in current context.
> >> UNSUPPORTED: gcc.dg/guality/vla-1.c  -O3 -flto -fno-use-linker-plugin \
> >>   -flto-partition=none line 17 i == 5
> >> 'a' has unknown type; cast it to its declared type
> >> UNSUPPORTED: gcc.dg/guality/vla-1.c  -O3 -flto -fno-use-linker-plugin \
> >>   -flto-partition=none line 17 sizeof (a) == 6
> >> ...
> >>
> >> Bootstrapped and reg-tested on x86_64.
> > 
> > This looks OK to me.
> 
> Committed.
> 
> >  Note that with a gdb with DW_OP_variable_value 
> > support we should be able to elide the VLA type in the concrete
> > instance...
> > 
> 
> Using this patch we elide the VLA type in the concrete instance for -O2
> -flto:
> ...
> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> index dd8f438dfd3..5776185d9c6 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -23712,12 +23712,14 @@ gen_variable_die (tree decl, tree origin,
> dw_die_ref context_die)
>   && variably_modified_type_p
>(type, decl_function_context (decl_or_origin)))
> {
> +#if 0
>   if (decl_by_reference_p (decl_or_origin))
> add_type_attribute (var_die, TREE_TYPE (type),
> TYPE_UNQUALIFIED, false,
> context_die);
>   else
> add_type_attribute (var_die, type,
> decl_quals (decl_or_origin),
> false, context_die);
> +#endif
> }
> 
>   goto gen_variable_die_location;
> ...
> and using master gdb (which supports DW_OP_variable_value, yay) we get:
> ...
> 7return a[0];  /* { dg-final { gdb-test . "sizeof (a)"
> "6" } } */
> vla-1.gdb:3: Error in sourced command file:
> value has been optimized out
> ...
> 
> When evaluating sizeof (a) in gdb:
> - we look for the concrete type die of a
> - since that one is elided, we fall back on the type die of the abstract
>   origin of a
> - that one has an expr location for the upper bound containing a
>   DW_OP_variable_value referring to a variable in early debug.
> - that variable has no DW_AT_location, so we end up with "value has been
>   optimized out"
> 
> AFAIU, that's roughly the issue discussed in
> PR20426 - "gdb does not interpret DWARF annotating imported units fully"
>  ( https://sourceware.org/bugzilla/show_bug.cgi?id=20426 ),.
> 
> Is my understanding correct that it's not yet clear how this should be
> fixed?

ISTR Jakub recently said in a bug or in a mail the debug consumer
should look

[PATCH, Darwin] Update LTO specs to match gcc/gcc.c

[Iain Sandoe]

Hi,

I committed the following as obvious.

thanks
Iain


 2018-08-22  Iain Sandoe  

gcc/
 
* config/darwin.h (LINK_COMMAND_SPEC_A): Sync LTO options with
the sequence used in gcc/gcc.c.

Index: gcc/config/darwin.h
===
--- gcc/config/darwin.h (revision 263769)
+++ gcc/config/darwin.h (working copy)
@@ -175,7 +175,7 @@
 %(linker)" \
 LINK_PLUGIN_SPEC \
 "%{flto*:%

Re: [PATCH] print full STRING_CST in Gimple dumps (PR 87052)

[Richard Biener]

On Wed, Aug 22, 2018 at 4:56 AM Martin Sebor  wrote:
>
> In the discussion of the fallout from the enhancement for pr71625
> it was pointed out that STRING_CSts in Gimple dumps extend only
> to the first nul and don't include any subsequent characters,
> and that this makes the dumps harder to read and might give rise
> to the question whether the code is correct.
>
> In the attached patch I enhance the pretty_print_string() function
> to print the full contents of a STRING_CST, including any embedded
> nuls to make the dumps clearer.  I got rid of the single digit
> escapes like '\1' since they make the string look ambiguous.
> If TREE_STRING_LENGTH (node) is guaranteed to be non-zero these
> days the test for it being so may be redundant but I figured it's
> better to be safe than sorry.
>
> A further enhancement might be to also distinguish the type of
> the STRING_CST.

And somehow indicate whether it is \0 terminated (just thinking of
the GIMPLE FE and how it parses string literals simply by relying
on libcpp).

Can you write a not \0 terminated string literal in C?

The patch is OK.

Thanks,
Richard.

>
> Martin
>

Re: [PATCH, driver specs][1] Put -flto-partition= on the collect2 c/l

[Iain Sandoe]

> On 20 Aug 2018, at 11:01, Richard Biener  wrote:
> 
> On Sat, Aug 18, 2018 at 9:00 PM Iain Sandoe  wrote:

>> While working on the Darwin LTO issues I noticed that collect2 looks for 
>> "-flto-partition=none” in its command line option, but it doesn’t get passed.
>> 
>> So - is the attached patch the right idea, or should collect2 be looking in 
>> the COLLECT_GCC_OPTIONS as the lto-wrapper does?
> 
> Looking at COLLECT_GCC_OPTIONS is probably better.

So I bootstrapped and tested the following on Darwin and Linux (both with a 32b 
multilib)

It’s kinda odd that we really have to look in two places for the union of the 
options to be handled, although the reasoning is clear enough.

Comments?
thanks
Iain

gcc/

* collect2.c (main): Combine flags from both the command line and
COLLECT_GCC_OPTIONS to determine the set in force.

---
 gcc/collect2.c | 55 +-
 1 file changed, 32 insertions(+), 23 deletions(-)

diff --git a/gcc/collect2.c b/gcc/collect2.c
index a96af137a4..9ead5a6a1d 100644
--- a/gcc/collect2.c
+++ b/gcc/collect2.c
@@ -979,13 +979,14 @@ main (int argc, char **argv)
   object = CONST_CAST2 (const char **, char **, object_lst);
 
 #ifdef DEBUG
-  debug = 1;
+  debug = true;
 #endif
 
-  /* Parse command line early for instances of -debug.  This allows
- the debug flag to be set before functions like find_a_file()
- are called.  We also look for the -flto or -flto-partition=none flag to 
know
- what LTO mode we are in.  */
+  save_temps = false;
+  verbose = false;
+  /* Parse command line / environment for flags we want early.
+ This allows the debug flag to be set before functions like find_a_file()
+ are called. */
   {
 bool no_partition = false;
 
@@ -993,8 +994,6 @@ main (int argc, char **argv)
   {
if (! strcmp (argv[i], "-debug"))
  debug = true;
-else if (! strcmp (argv[i], "-flto-partition=none"))
- no_partition = true;
else if (!strncmp (argv[i], "-fno-lto", 8))
  lto_mode = LTO_MODE_NONE;
 else if (! strcmp (argv[i], "-plugin"))
@@ -1027,13 +1026,6 @@ main (int argc, char **argv)
aixlazy_flag = 1;
 #endif
   }
-verbose = debug;
-find_file_set_debug (debug);
-if (use_plugin)
-  lto_mode = LTO_MODE_NONE;
-if (no_partition && lto_mode == LTO_MODE_WHOPR)
-  lto_mode = LTO_MODE_LTO;
-  }
 
 #ifndef DEFAULT_A_OUT_NAME
   output_file = "a.out";
@@ -1041,20 +1033,37 @@ main (int argc, char **argv)
   output_file = DEFAULT_A_OUT_NAME;
 #endif
 
-  obstack_begin (&temporary_obstack, 0);
-  temporary_firstobj = (char *) obstack_alloc (&temporary_obstack, 0);
+obstack_begin (&temporary_obstack, 0);
+temporary_firstobj = (char *) obstack_alloc (&temporary_obstack, 0);
 
 #ifndef HAVE_LD_DEMANGLE
   current_demangling_style = auto_demangling;
 #endif
-  p = getenv ("COLLECT_GCC_OPTIONS");
-  while (p && *p)
-{
-  const char *q = extract_string (&p);
-  if (*q == '-' && (q[1] == 'm' || q[1] == 'f'))
-   num_c_args++;
+
+/* Now pick up any flags we want early from COLLECT_GCC_OPTIONS
+   The LTO options are passed here as are other options that might
+   be unsuitable for ld (e.g. -save-temps).  */
+p = getenv ("COLLECT_GCC_OPTIONS");
+while (p && *p)
+  {
+   const char *q = extract_string (&p);
+   if (*q == '-' && (q[1] == 'm' || q[1] == 'f'))
+ num_c_args++;
+   if (strncmp (q, "-flto-partition=none", 20) == 0)
+ no_partition = true;
+   else if (strncmp (q, "-fno-lto", 8) == 0)
+ lto_mode = LTO_MODE_NONE;
 }
-  obstack_free (&temporary_obstack, temporary_firstobj);
+obstack_free (&temporary_obstack, temporary_firstobj);
+
+verbose |= debug;
+save_temps |= debug;
+find_file_set_debug (debug);
+if (use_plugin)
+  lto_mode = LTO_MODE_NONE;
+if (no_partition && lto_mode == LTO_MODE_WHOPR)
+  lto_mode = LTO_MODE_LTO;
+  }
 
   /* -fno-profile-arcs -fno-test-coverage -fno-branch-probabilities
  -fno-exceptions -w -fno-whole-program */
-- 
2.17.1

Re: [PATCH, driver specs][2] Put -flto-partition= on the collect2 c/l

[Iain Sandoe]

> On 20 Aug 2018, at 11:01, Richard Biener  wrote:
> 
> On Sat, Aug 18, 2018 at 9:00 PM Iain Sandoe  wrote:
>> 

>> I plan on making Darwin default to fno-lto for link unless there’s an 
>> “flto*” on the link line, since otherwise there’s a process launch for every 
>> object on the c/l to do “nm”, which is quite heavy weight.  At present, ISTM 
>> if the compiler is configured with —enable-lto, the link line defaults to 
>> assuming that every object needs to be checked.
> 
> I think we wanted to transparently handle LTO objects (similar to how
> it works with a linker plugin).

At present, we are not only calling nm for every object, but also dsymutil 
sometimes unnecessarily - since the assumption on the “maybe_lto” path is that 
an temporary file will be generated.

as a headline - when I did the simplistic “default is to assume fno-lto” that 
took 10% off the bootstrap time (with a stage#1 compiler without the 
optimisation).

- I have a patch under test as below + better fidelity of avoiding dsymutil 
runs.

> Ideally collect2 wouldn't use nm but simple-object to inspect files
> (but that doesn't have symbol table query support

Hrm.my WIP had grow symtab awareness to support debug-section copy on mach-o
(and the ELF impl. looks like it understands both symtab and relocs) 
- so maybe that’s not as far away as we might think.

> though looking for LTO specific sections would have been better in the
> first place - I'd suggest .gnu.lto_.symtab).

I’ve done this noting that the symtab seems to be the last emitted section - is 
that why you chose it (for security, rather than just stopping as soon as we 
see a .gnu.lto_. section?)

bootstrapped (Darwin and Linux) along with a third patch to make collect2 
respond to -save-temps.

comments?
thanks
Iain

gcc/

* collect2.c (maybe_run_lto_and_relink): Don’t say we have a temp file
unless we actually did some LTO.
(has_lto_symtab, is_lto_object_file): New.
(maybe_lto_object_file): Remove.
(scan_prog_file): Use is_lto_object_file() instead of scanning the 
output
of nm.

---
 gcc/collect2.c | 116 ++---
 1 file changed, 53 insertions(+), 63 deletions(-)

diff --git a/gcc/collect2.c b/gcc/collect2.c
index 9ead5a6a1d..4b0f191ad3 100644
--- a/gcc/collect2.c
+++ b/gcc/collect2.c
@@ -30,6 +30,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "tm.h"
 #include "filenames.h"
 #include "file-find.h"
+#include "simple-object.h"
+#include "lto-section-names.h"
 
 /* TARGET_64BIT may be defined to use driver specific functionality. */
 #undef TARGET_64BIT
@@ -804,7 +806,9 @@ maybe_run_lto_and_relink (char **lto_ld_argv, char 
**object_lst,
   /* Run the linker again, this time replacing the object files
  optimized by the LTO with the temporary file generated by the LTO.  */
   fork_execute ("ld", out_lto_ld_argv, HAVE_GNU_LD && at_file_supplied);
-  post_ld_pass (true);
+  /* We assume that temp files were created, and therefore we need to take
+ that into account (maybe run dsymutil).  */
+  post_ld_pass (/*temp_file*/true);
   free (lto_ld_argv);
 
   maybe_unlink_list (lto_o_files);
@@ -814,10 +818,11 @@ maybe_run_lto_and_relink (char **lto_ld_argv, char 
**object_lst,
   /* Our caller is relying on us to do the link
  even though there is no LTO back end work to be done.  */
   fork_execute ("ld", lto_ld_argv, HAVE_GNU_LD && at_file_supplied);
-  post_ld_pass (false);
+  /* No LTO objects were found, so no new temp file.  */
+  post_ld_pass (/*temp_file*/false);
 }
   else
-post_ld_pass (true);
+post_ld_pass (false); /* No LTO objects were found, no temp file.  */
 }
 
 /* Main program.  */
@@ -1677,7 +1682,7 @@ main (int argc, char **argv)
if (lto_mode != LTO_MODE_NONE)
  maybe_run_lto_and_relink (ld1_argv, object_lst, object, false);
else
- post_ld_pass (false);
+ post_ld_pass (/*temp_file*/false);
 
maybe_unlink (c_file);
maybe_unlink (o_file);
@@ -1749,7 +1754,7 @@ main (int argc, char **argv)
 #ifdef COLLECT_EXPORT_LIST
   maybe_unlink (export_file);
 #endif
-  post_ld_pass (false);
+  post_ld_pass (/*temp_file*/false);
 
   maybe_unlink (c_file);
   maybe_unlink (o_file);
@@ -1843,7 +1848,7 @@ main (int argc, char **argv)
   else
 {
   fork_execute ("ld", ld2_argv, HAVE_GNU_LD && at_file_supplied);
-  post_ld_pass (false);
+  post_ld_pass (/*temp_file*/false);
 }
 
   /* Let scan_prog_file do any final mods (OSF/rose needs this for
@@ -2300,38 +2305,48 @@ write_aix_file (FILE *stream, struct id *list)
 
 /* Check to make sure the file is an LTO object file.  */
 
+static int
+has_lto_symtab (void *data, const char *name ATTRIBUTE_UNUSED,
+   off_t offset ATTRIBUTE_UNUSED,
+   off_t length ATTRIBUTE_UNUSED)
+{
+  int *found = (int *) data;
+
+  if (str

Re: Make the vectoriser drop to strided accesses for stores with gaps

[Richard Sandiford]

Richard Biener  writes:
> On Fri, Jul 20, 2018 at 12:57 PM Richard Sandiford
>  wrote:
>>
>> We could vectorise:
>>
>>  for (...)
>>{
>>  a[0] = ...;
>>  a[1] = ...;
>>  a[2] = ...;
>>  a[3] = ...;
>>  a += stride;
>>}
>>
>> (including the case when stride == 8) but not:
>>
>>  for (...)
>>{
>>  a[0] = ...;
>>  a[1] = ...;
>>  a[2] = ...;
>>  a[3] = ...;
>>  a += 8;
>>}
>>
>> (where the stride is always 8).  The former was treated as a "grouped
>> and strided" store, while the latter was treated as grouped store with
>> gaps, which we don't support.
>>
>> This patch makes us treat groups of stores with gaps at the end as
>> strided groups too.  I tried to go through all uses of STMT_VINFO_STRIDED_P
>> and all vector uses of DR_STEP to see whether there were any hard-baked
>> assumptions, but couldn't see any.  I wondered whether we should relax:
>>
>>   /* We do not have to consider dependences between accesses that belong
>>  to the same group, unless the stride could be smaller than the
>>  group size.  */
>>   if (DR_GROUP_FIRST_ELEMENT (stmtinfo_a)
>>   && (DR_GROUP_FIRST_ELEMENT (stmtinfo_a)
>>   == DR_GROUP_FIRST_ELEMENT (stmtinfo_b))
>>   && !STMT_VINFO_STRIDED_P (stmtinfo_a))
>> return false;
>>
>> for cases in which the step is constant and the absolute step is known
>> to be greater than the group size, but data dependence analysis should
>> already return chrec_known for those cases.
>>
>> The new test is a version of vect-avg-15.c with the variable step
>> replaced by a constant one.
>>
>> A natural follow-on would be to do the same for groups with gaps in
>> the middle:
>>
>>   /* Check that the distance between two accesses is equal to the 
>> type
>>  size. Otherwise, we have gaps.  */
>>   diff = (TREE_INT_CST_LOW (DR_INIT (data_ref))
>>   - TREE_INT_CST_LOW (prev_init)) / type_size;
>>   if (diff != 1)
>> {
>>   [...]
>>   if (DR_IS_WRITE (data_ref))
>> {
>>   if (dump_enabled_p ())
>> dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
>>  "interleaved store with gaps\n");
>>   return false;
>> }
>>
>> But I think we should do that separately and see what the fallout
>> from this change is first.
>
> Agreed.
>
>> Tested on aarch64-linux-gnu (with and without SVE), aarch64_be-elf
>> and x86_64-linux-gnu.  OK to install?
>
> Do you need to set STMT_VINFO_STRIDED_P on all stmts?  I think
> it is enough to set it on the first group element.
>
> OK otherwise.

Thanks.  Here's what I installed after fixing the STMT_VINFO_STRIDED_P
uses in get_load_store_type.

Richard


2018-08-22  Richard Sandiford  

gcc/
* tree-vect-data-refs.c (vect_analyze_group_access_1): Convert
grouped stores with gaps to a strided group.

gcc/testsuite/
* gcc.dg/vect/vect-avg-16.c: New test.
* gcc.dg/vect/slp-37.c: Expect the loop to be vectorized.
* gcc.dg/vect/vect-strided-u8-i8-gap4.c,
* gcc.dg/vect/vect-strided-u8-i8-gap4-big-array.c: Likewise for
the second loop in main1.

Index: gcc/tree-vect-data-refs.c
===
--- gcc/tree-vect-data-refs.c   2018-08-21 14:47:07.795167843 +0100
+++ gcc/tree-vect-data-refs.c   2018-08-22 10:24:22.070479189 +0100
@@ -2633,10 +2633,8 @@ vect_analyze_group_access_1 (dr_vec_info
   if (groupsize != count
  && !DR_IS_READ (dr))
 {
- if (dump_enabled_p ())
-   dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
-"interleaved store with gaps\n");
- return false;
+ groupsize = count;
+ STMT_VINFO_STRIDED_P (stmt_info) = true;
}
 
   /* If there is a gap after the last load in the group it is the
@@ -2652,6 +2650,8 @@ vect_analyze_group_access_1 (dr_vec_info
   "Detected interleaving ");
  if (DR_IS_READ (dr))
dump_printf (MSG_NOTE, "load ");
+ else if (STMT_VINFO_STRIDED_P (stmt_info))
+   dump_printf (MSG_NOTE, "strided store ");
  else
dump_printf (MSG_NOTE, "store ");
  dump_printf (MSG_NOTE, "of size %u starting with ",
Index: gcc/testsuite/gcc.dg/vect/vect-avg-16.c
===
--- /dev/null   2018-07-26 10:26:13.137955424 +0100
+++ gcc/testsuite/gcc.dg/vect/vect-avg-16.c 2018-08-22 10:24:22.070479189 
+0100
@@ -0,0 +1,52 @@
+/* { dg-additional-options "-O3" } */
+/* { dg-require-effective-target vect_int } */
+
+#include "tree-vect.h"
+
+#define N 80
+
+void __attribute__ ((noipa))
+f (signed char *restrict a, signed char *restrict b,
+   signed char *restrict c, int

Re: [PATCH, driver specs][3] Put -flto-partition= on the collect2 c/l

[Iain Sandoe]

> On 20 Aug 2018, at 11:01, Richard Biener  wrote:
> 
> On Sat, Aug 18, 2018 at 9:00 PM Iain Sandoe  wrote:
>> 
>> Hi
>> 
>> While working on the Darwin LTO issues I noticed that collect2 looks for 
>> "-flto-partition=none” in its command line option, but it doesn’t get passed.
>> 
>> So - is the attached patch the right idea, or should collect2 be looking in 
>> the COLLECT_GCC_OPTIONS as the lto-wrapper does?
> 
> Looking at COLLECT_GCC_OPTIONS is probably better.

related;  
collect2 is currently “non-obvious” in that it doesn’t respond to -save-temps 
and to get the tempories saved we have to invoke “-debug” which produces a 
bunch of other output we probably don’t care about most times.

So .. perhaps we could move to 
 -save-temps (does the “usual thing”)
 -v (does the “usual thing”)
 -debug imples the two above + other informative output

(it would be kinda nice if there was some way to implement -### so that we 
could see the collect subordinate pass structure)

comments?
thanks
Iain

gcc/
* collect2.c (main): Parse the output file early so we can make nicer
temp names.  Respond to “-save-temps” in the GCC OPTIONS.
(maybe_unlink): Don’t print “[Leaving…”] for files we never created
and don’t exist.

---
 gcc/collect2.c | 57 +-
 1 file changed, 43 insertions(+), 14 deletions(-)

diff --git a/gcc/collect2.c b/gcc/collect2.c
index 4b0f191ad3..b892e1a2aa 100644
--- a/gcc/collect2.c
+++ b/gcc/collect2.c
@@ -205,8 +205,8 @@ bool helpflag;  /* true if --help */
 static int shared_obj; /* true if -shared */
 static int static_obj; /* true if -static */
 
-static const char *c_file; /* .c for constructor/destructor 
list.  */
-static const char *o_file; /* .o for constructor/destructor 
list.  */
+static char *c_file;   /* .c for constructor/destructor list.  */
+static char *o_file;   /* .o for constructor/destructor list.  */
 #ifdef COLLECT_EXPORT_LIST
 static const char *export_file;/* .x for AIX export list. 
 */
 #endif
@@ -989,6 +989,13 @@ main (int argc, char **argv)
 
   save_temps = false;
   verbose = false;
+
+#ifndef DEFAULT_A_OUT_NAME
+  output_file = "a.out";
+#else
+  output_file = DEFAULT_A_OUT_NAME;
+#endif
+
   /* Parse command line / environment for flags we want early.
  This allows the debug flag to be set before functions like find_a_file()
  are called. */
@@ -1011,7 +1018,17 @@ main (int argc, char **argv)
  selected_linker = USE_BFD_LD;
else if (strcmp (argv[i], "-fuse-ld=gold") == 0)
  selected_linker = USE_GOLD_LD;
-
+   else if (strncmp (argv[i], "-o", 2) == 0)
+ {
+   /* Parse the output filename if it's given so that we can make
+  meaningful temp filenames.  */
+   if (argv[i][2] == '\0')
+ output_file = argv[i+1];
+   else if (argv[i][2] == '=')
+ output_file = &argv[i][3];
+   else
+ output_file = &argv[i][2];
+ }
 #ifdef COLLECT_EXPORT_LIST
/* These flags are position independent, although their order
   is important - subsequent flags override earlier ones. */
@@ -1032,12 +1049,6 @@ main (int argc, char **argv)
 #endif
   }
 
-#ifndef DEFAULT_A_OUT_NAME
-  output_file = "a.out";
-#else
-  output_file = DEFAULT_A_OUT_NAME;
-#endif
-
 obstack_begin (&temporary_obstack, 0);
 temporary_firstobj = (char *) obstack_alloc (&temporary_obstack, 0);
 
@@ -1058,6 +1069,9 @@ main (int argc, char **argv)
  no_partition = true;
else if (strncmp (q, "-fno-lto", 8) == 0)
  lto_mode = LTO_MODE_NONE;
+   else if (strncmp (q, "-save-temps", 11) == 0)
+ /* FIXME: Honour =obj.  */
+ save_temps = true;
 }
 obstack_free (&temporary_obstack, temporary_firstobj);
 
@@ -1217,8 +1231,22 @@ main (int argc, char **argv)
   *ld1++ = *ld2++ = ld_file_name;
 
   /* Make temp file names.  */
-  c_file = make_temp_file (".c");
-  o_file = make_temp_file (".o");
+  if (save_temps)
+{
+  c_file = (char *) xmalloc (strlen (output_file)
+ + sizeof (".cdtor.c") + 1);
+  strcpy (c_file, output_file);
+  strcat (c_file, ".cdtor.c");
+  o_file = (char *) xmalloc (strlen (output_file)
+ + sizeof (".cdtor.o") + 1);
+  strcpy (o_file, output_file);
+  strcat (o_file, ".cdtor.o");
+}
+  else
+{
+  c_file = make_temp_file (".cdtor.c");
+  o_file = make_temp_file (".cdtor.o");
+}
 #ifdef COLLECT_EXPORT_LIST
   export_file = make_temp_file (".x");
 #endif
@@ -1227,6 +1255,7 @@ main (int argc, char **argv)
   ldout = make_temp_file (".ld");
   lderrout = make_temp_file (".le");
 }
+  /* Build the command line to compile the ctor/dtor list.  */
   *c_ptr++ = c_file_name;
   *c_ptr++ = "-x";
   *c_ptr++ = 

Re: [PATCH] Make strlen range computations more conservative

[Bernd Edlinger]

Hi,

this is an update (v5) of my patch:

As discussed earlier, this version does no longer enable
-fassume-zero-terminated-char-arrays with -Ofast.

I am ready to remove the -fassume-zero-terminated-char-arrays altogether if we 
decide what
to do with the code-gen test cases that still use it (xfail or remove).


Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.
gcc:
2018-08-21  Bernd Edlinger  

* common.opt: Add new optimization option
-fassume-zero-terminated-char-arrays.
* gimple-fold.h (looks_like_a_char_array_without_typecast_p): Declare.
* gimple-fold.c (looks_like_a_char_array_without_typecast_p): Helper
function for strlen range estimations.
(get_range_strlen): Use looks_like_a_char_array_without_typecast_p.
* tree-ssa-strlen.c (maybe_set_strlen_range): Likewise.
(get_min_string_length): Avoid not NUL terminated string literals.
* doc/invoke.texi: Document -fassume-zero-terminated-char-arrays.
* tree-ssa-dse.c (compute_trims): Avoid folding away undefined
behaviour.

testsuite:
2018-08-32  Bernd Edlinger  

* gcc.dg/pr83373.c: Add xfail.
* gcc.dg/strlenopt-36.c: Adjust test expectations.
* gcc.dg/strlenopt-40.c: Likewise.
* gcc.dg/strlenopt-45.c: Likewise.
* gcc.dg/strlenopt-48.c: Likewise.
* gcc.dg/strlenopt-51.c: Likewise.
* gcc.dg/strlenopt-57.c: New test.
* gcc.dg/strlenopt-58.c: New test.
* gcc.dg/strlenopt-59.c: New test.

diff -Npur gcc/common.opt gcc/common.opt
--- gcc/common.opt	2018-08-19 17:11:34.0 +0200
+++ gcc/common.opt	2018-08-22 09:04:53.520305828 +0200
@@ -1025,6 +1025,10 @@ fsanitize-undefined-trap-on-error
 Common Driver Report Var(flag_sanitize_undefined_trap_on_error) Init(0)
 Use trap instead of a library function for undefined behavior sanitization.
 
+fassume-zero-terminated-char-arrays
+Common Var(flag_assume_zero_terminated_char_arrays) Optimization Init(0)
+Optimize under the assumption that char arrays must always be zero terminated.
+
 fasynchronous-unwind-tables
 Common Report Var(flag_asynchronous_unwind_tables) Optimization
 Generate unwind tables that are exact at each instruction boundary.
diff -Npur gcc/doc/invoke.texi gcc/doc/invoke.texi
--- gcc/doc/invoke.texi	2018-08-21 10:13:34.0 +0200
+++ gcc/doc/invoke.texi	2018-08-22 09:06:18.645102845 +0200
@@ -388,7 +388,8 @@ Objective-C and Objective-C++ Dialects}.
 -falign-jumps[=@var{n}[:@var{m}:[@var{n2}[:@var{m2} @gol
 -falign-labels[=@var{n}[:@var{m}:[@var{n2}[:@var{m2} @gol
 -falign-loops[=@var{n}[:@var{m}:[@var{n2}[:@var{m2} @gol
--fassociative-math  -fauto-profile  -fauto-profile[=@var{path}] @gol
+-fassociative-math  -fassume-zero-terminated-char-arrays @gol
+-fauto-profile  -fauto-profile[=@var{path}] @gol
 -fauto-inc-dec  -fbranch-probabilities @gol
 -fbranch-target-load-optimize  -fbranch-target-load-optimize2 @gol
 -fbtr-bb-exclusive  -fcaller-saves @gol
@@ -9978,6 +9979,16 @@ is automatically enabled when both @opti
 
 The default is @option{-fno-associative-math}.
 
+@item -fassume-zero-terminated-char-arrays
+@opindex fassume-zero-terminated-char-arrays
+
+Optimize under the assumption that char arrays must always be zero
+terminated.  This may have an effect on code that uses strlen to
+check the string length, for instance in assertions.  Under certain
+conditions such checks can be optimized away.
+
+The default is @option{-fno-assume-zero-terminated-char-arrays}.
+
 @item -freciprocal-math
 @opindex freciprocal-math
 
diff -Npur gcc/gimple-fold.c gcc/gimple-fold.c
--- gcc/gimple-fold.c	2018-08-19 17:11:34.0 +0200
+++ gcc/gimple-fold.c	2018-08-22 09:04:53.741302702 +0200
@@ -1257,6 +1257,45 @@ gimple_fold_builtin_memset (gimple_stmt_
   return true;
 }
 
+/* Determine if a char array is suitable for strlen range estimations.
+   Return false if ARG is not a char array, or if the inner reference
+   chain appears to go through a type cast, or if !optimistic,
+   or if !flag_assume_zero_terminated_char_arrays.
+   Otherwise return true.
+   Note that type gimple type informations are not 100% guaranteed
+   to be accurate.
+   OPTIMISTIC is true when the result is used for warnings only.  */
+
+bool
+looks_like_a_char_array_without_typecast_p (tree arg, bool optimistic)
+{
+  if (!flag_assume_zero_terminated_char_arrays && !optimistic)
+return false;
+
+  /* We handle arrays of integer types.  */
+  if (TREE_CODE (TREE_TYPE (arg)) != ARRAY_TYPE
+  || TREE_CODE (TREE_TYPE (TREE_TYPE (arg))) != INTEGER_TYPE
+  || TYPE_MODE (TREE_TYPE (TREE_TYPE (arg))) != TYPE_MODE (char_type_node)
+  || TYPE_PRECISION (TREE_TYPE (TREE_TYPE (arg)))
+	 != TYPE_PRECISION (char_type_node))
+return false;
+
+  tree base = arg;
+  while (TREE_CODE (base) == ARRAY_REF
+	 || TREE_CODE (base) == ARRAY_RANGE_REF
+	 || TREE_CODE (base) == COMPONENT_REF)
+base = TREE_OPERAND (ba

[PATCH] Change AArch64 specific FMAX/FMIN tests into generic MAX_EXPR/MIN_EXPR tests

[Szabolcs Nagy]

gfortran now always uses MAX_EXPR/MIN_EXPR for MAX/MIN intrinsics, so the
AArch64 specific FMAX/FMIN tests are no longer valid.

2018-08-22  Szabolcs Nagy  

* gfortran.dg/max_fmax_aarch64.f90: Rename to...
* gfortran.dg/max_expr.f90: ...this.
* gfortran.dg/min_fmin_aarch64.f90: Rename to...
* gfortran.dg/min_expr.f90: ...this.
diff --git a/gcc/testsuite/gfortran.dg/max_fmax_aarch64.f90 b/gcc/testsuite/gfortran.dg/max_expr.f90
similarity index 74%
rename from gcc/testsuite/gfortran.dg/max_fmax_aarch64.f90
rename to gcc/testsuite/gfortran.dg/max_expr.f90
index b818241a1f9..c00ad62f744 100644
--- a/gcc/testsuite/gfortran.dg/max_fmax_aarch64.f90
+++ b/gcc/testsuite/gfortran.dg/max_expr.f90
@@ -1,4 +1,4 @@
-! { dg-do compile { target aarch64*-*-* } }
+! { dg-do compile }
 ! { dg-options "-O2 -fdump-tree-optimized" }
 
 subroutine foo (a, b, c, d, e, f, g, h)
@@ -12,4 +12,4 @@ subroutine foof (a, b, c, d, e, f, g, h)
 end subroutine
 
 
-! { dg-final { scan-tree-dump-times "\.FMAX " 14 "optimized" } }
+! { dg-final { scan-tree-dump-times "MAX_EXPR " 14 "optimized" } }
diff --git a/gcc/testsuite/gfortran.dg/min_fmin_aarch64.f90 b/gcc/testsuite/gfortran.dg/min_expr.f90
similarity index 74%
rename from gcc/testsuite/gfortran.dg/min_fmin_aarch64.f90
rename to gcc/testsuite/gfortran.dg/min_expr.f90
index 009869b497d..5f32d5073c9 100644
--- a/gcc/testsuite/gfortran.dg/min_fmin_aarch64.f90
+++ b/gcc/testsuite/gfortran.dg/min_expr.f90
@@ -1,4 +1,4 @@
-! { dg-do compile { target aarch64*-*-* } }
+! { dg-do compile }
 ! { dg-options "-O2 -fdump-tree-optimized" }
 
 subroutine foo (a, b, c, d, e, f, g, h)
@@ -12,4 +12,4 @@ subroutine foof (a, b, c, d, e, f, g, h)
   a = min (a, b, c, d, e, f, g, h)
 end subroutine
 
-! { dg-final { scan-tree-dump-times "\.FMIN " 14 "optimized" } }
+! { dg-final { scan-tree-dump-times "MIN_EXPR " 14 "optimized" } }

Re: [PATCH, driver specs][1] Put -flto-partition= on the collect2 c/l

[Richard Biener]

On Wed, Aug 22, 2018 at 2:51 PM Iain Sandoe  wrote:
>
>
> > On 20 Aug 2018, at 11:01, Richard Biener  wrote:
> >
> > On Sat, Aug 18, 2018 at 9:00 PM Iain Sandoe  wrote:
>
> >> While working on the Darwin LTO issues I noticed that collect2 looks for 
> >> "-flto-partition=none” in its command line option, but it doesn’t get 
> >> passed.
> >>
> >> So - is the attached patch the right idea, or should collect2 be looking 
> >> in the COLLECT_GCC_OPTIONS as the lto-wrapper does?
> >
> > Looking at COLLECT_GCC_OPTIONS is probably better.
>
> So I bootstrapped and tested the following on Darwin and Linux (both with a 
> 32b multilib)
>
> It’s kinda odd that we really have to look in two places for the union of the 
> options to be handled, although the reasoning is clear enough.
>
> Comments?

OK.

Richard.

> thanks
> Iain
>
> gcc/
>
> * collect2.c (main): Combine flags from both the command line and
> COLLECT_GCC_OPTIONS to determine the set in force.
>
> ---
>  gcc/collect2.c | 55 +-
>  1 file changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/gcc/collect2.c b/gcc/collect2.c
> index a96af137a4..9ead5a6a1d 100644
> --- a/gcc/collect2.c
> +++ b/gcc/collect2.c
> @@ -979,13 +979,14 @@ main (int argc, char **argv)
>object = CONST_CAST2 (const char **, char **, object_lst);
>
>  #ifdef DEBUG
> -  debug = 1;
> +  debug = true;
>  #endif
>
> -  /* Parse command line early for instances of -debug.  This allows
> - the debug flag to be set before functions like find_a_file()
> - are called.  We also look for the -flto or -flto-partition=none flag to 
> know
> - what LTO mode we are in.  */
> +  save_temps = false;
> +  verbose = false;
> +  /* Parse command line / environment for flags we want early.
> + This allows the debug flag to be set before functions like find_a_file()
> + are called. */
>{
>  bool no_partition = false;
>
> @@ -993,8 +994,6 @@ main (int argc, char **argv)
>{
> if (! strcmp (argv[i], "-debug"))
>   debug = true;
> -else if (! strcmp (argv[i], "-flto-partition=none"))
> - no_partition = true;
> else if (!strncmp (argv[i], "-fno-lto", 8))
>   lto_mode = LTO_MODE_NONE;
>  else if (! strcmp (argv[i], "-plugin"))
> @@ -1027,13 +1026,6 @@ main (int argc, char **argv)
> aixlazy_flag = 1;
>  #endif
>}
> -verbose = debug;
> -find_file_set_debug (debug);
> -if (use_plugin)
> -  lto_mode = LTO_MODE_NONE;
> -if (no_partition && lto_mode == LTO_MODE_WHOPR)
> -  lto_mode = LTO_MODE_LTO;
> -  }
>
>  #ifndef DEFAULT_A_OUT_NAME
>output_file = "a.out";
> @@ -1041,20 +1033,37 @@ main (int argc, char **argv)
>output_file = DEFAULT_A_OUT_NAME;
>  #endif
>
> -  obstack_begin (&temporary_obstack, 0);
> -  temporary_firstobj = (char *) obstack_alloc (&temporary_obstack, 0);
> +obstack_begin (&temporary_obstack, 0);
> +temporary_firstobj = (char *) obstack_alloc (&temporary_obstack, 0);
>
>  #ifndef HAVE_LD_DEMANGLE
>current_demangling_style = auto_demangling;
>  #endif
> -  p = getenv ("COLLECT_GCC_OPTIONS");
> -  while (p && *p)
> -{
> -  const char *q = extract_string (&p);
> -  if (*q == '-' && (q[1] == 'm' || q[1] == 'f'))
> -   num_c_args++;
> +
> +/* Now pick up any flags we want early from COLLECT_GCC_OPTIONS
> +   The LTO options are passed here as are other options that might
> +   be unsuitable for ld (e.g. -save-temps).  */
> +p = getenv ("COLLECT_GCC_OPTIONS");
> +while (p && *p)
> +  {
> +   const char *q = extract_string (&p);
> +   if (*q == '-' && (q[1] == 'm' || q[1] == 'f'))
> + num_c_args++;
> +   if (strncmp (q, "-flto-partition=none", 20) == 0)
> + no_partition = true;
> +   else if (strncmp (q, "-fno-lto", 8) == 0)
> + lto_mode = LTO_MODE_NONE;
>  }
> -  obstack_free (&temporary_obstack, temporary_firstobj);
> +obstack_free (&temporary_obstack, temporary_firstobj);
> +
> +verbose |= debug;
> +save_temps |= debug;
> +find_file_set_debug (debug);
> +if (use_plugin)
> +  lto_mode = LTO_MODE_NONE;
> +if (no_partition && lto_mode == LTO_MODE_WHOPR)
> +  lto_mode = LTO_MODE_LTO;
> +  }
>
>/* -fno-profile-arcs -fno-test-coverage -fno-branch-probabilities
>   -fno-exceptions -w -fno-whole-program */
> --
> 2.17.1
>
>

Re: [PATCH] Call braced_list_to_string after array size is fixed

[Bernd Edlinger]

Hi,

this is a minor update to the previous version, which does no longer try
to fold the array initializer of VLAs, due to this initializer hitting
an assertion in my varasm patch, I believe it is certainly not worth the
risk to do that optimization with VLAs.

Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.

On 08/17/18 14:01, Bernd Edlinger wrote:
> Hi,
> 
> Yes I know Martin will see this as a capital offense, but to my excuse I must 
> say
> it only happened in self defense :-)
> 
> As my other patch series depends on STRNG_CSTs with certain properties, it got
> broken by the way the string constants are created now, due to the 71625 
> patch.
> 
> In order to be able to bootstrap my patch with current trunk, I had to move
> the point where the char array constructor is converted to a STRING_CST to the
> point directly after the array bounds are fixed, so that it is possible to 
> create
> the string constant (with implicit NUL termination) like all other C strings, 
> without
> making the array larger.
> 
> I added a test case for a crash with an invalid string intializer (in C++).
> and removed an xfail in the new test case string2.C.  This way the C++ 
> diagnostics
> matches the state before 71625 was applied.
> 
> This does not fix the other problems with 71625, but I don't think they are 
> related
> to the way the string constants are created, so there should be no conflict.
> 
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
> 
> 
> Thanks
> Bernd.
c-family:
2018-08-17  Bernd Edlinger  

	* c-common.c (braced_list_to_string): Remove eval parameter.
	Add some more checks.  Always create zero-terminated STRING_CST.
	* c-common.h (braced_list_to_string): Adjust prototype.

c:
2018-08-17  Bernd Edlinger  

	* c-decl.c (finish_decl): Call braced_list_to_string here ...
	* c-parser.c (c_parser_declaration_or_fndef): ... instead of here.


cp:
2018-08-17  Bernd Edlinger  

	* decl.c (eval_check_narrowing): Remove.
	(check_initializer): Move call to braced_list_to_string from here ...
	* typeck2.c (store_init_value): ... to here.
	(digest_init_r): Remove handing of signed/unsigned char strings.

testsuite:
2018-08-17  Bernd Edlinger  

	* c-c++-common/array-init.c: New test.
	* g++.dg/init/string2.C: Remove xfail.


Index: gcc/c/c-decl.c
===
--- gcc/c/c-decl.c	(revision 263554)
+++ gcc/c/c-decl.c	(working copy)
@@ -5011,6 +5011,17 @@ finish_decl (tree decl, location_t init_
   relayout_decl (decl);
 }
 
+  if (TREE_CODE (type) == ARRAY_TYPE
+  && DECL_INITIAL (decl)
+  && TREE_CODE (DECL_INITIAL (decl)) == CONSTRUCTOR)
+{
+  tree typ1 = TYPE_MAIN_VARIANT (TREE_TYPE (type));
+  if (typ1 == char_type_node
+	  || typ1 == signed_char_type_node
+	  || typ1 == unsigned_char_type_node)
+	DECL_INITIAL (decl) = braced_list_to_string (type, DECL_INITIAL (decl));
+}
+
   if (VAR_P (decl))
 {
   if (init && TREE_CODE (init) == CONSTRUCTOR)
Index: gcc/c/c-parser.c
===
--- gcc/c/c-parser.c	(revision 263554)
+++ gcc/c/c-parser.c	(working copy)
@@ -2126,15 +2126,6 @@ c_parser_declaration_or_fndef (c_parser
 	  if (d != error_mark_node)
 		{
 		  maybe_warn_string_init (init_loc, TREE_TYPE (d), init);
-
-		  /* Try to convert a string CONSTRUCTOR into a STRING_CST.  */
-		  tree valtype = TREE_TYPE (init.value);
-		  if (TREE_CODE (init.value) == CONSTRUCTOR
-		  && TREE_CODE (valtype) == ARRAY_TYPE
-		  && TYPE_STRING_FLAG (TREE_TYPE (valtype)))
-		if (tree str = braced_list_to_string (valtype, init.value))
-		  init.value = str;
-
 		  finish_decl (d, init_loc, init.value,
 			   init.original_type, asm_name);
 		}
Index: gcc/c-family/c-common.c
===
--- gcc/c-family/c-common.c	(revision 263554)
+++ gcc/c-family/c-common.c	(working copy)
@@ -8510,39 +8510,24 @@ maybe_add_include_fixit (rich_location *
 }
 
 /* Attempt to convert a braced array initializer list CTOR for array
-   TYPE into a STRING_CST for convenience and efficiency.  When non-null,
-   use EVAL to attempt to evalue constants (used by C++).  Return
-   the converted string on success or null on failure.  */
+   TYPE into a STRING_CST for convenience and efficiency.  Return
+   the converted string on success or the original ctor on failure.  */
 
 tree
-braced_list_to_string (tree type, tree ctor, tree (*eval)(tree, tree))
+braced_list_to_string (tree type, tree ctor)
 {
-  unsigned HOST_WIDE_INT nelts = CONSTRUCTOR_NELTS (ctor);
+  if (!tree_fits_uhwi_p (TYPE_SIZE_UNIT (type)))
+return ctor;
 
   /* If the array has an explicit bound, use it to constrain the size
  of the string.  If it doesn't, be sure to create a string that's
  as long as implied by the index of the last zero specified via
  a designator, as in:

[PATCH] combine: Do another check before splitting a parallel (PR86771)

[Segher Boessenkool]

When combine splits a resulting parallel into its two SETs, it has to
place one at i2, and the other stays at i3.  This does not work if the
destination of the SET that will be placed at i2 is modified between
i2 and i3.  This patch fixes it.

Tested on powerpc64-linux {-m32,-m64}, and the reported failing fortran
testcase on an arm-linux-gnueabi crosscompiler (by inspection).

Committing.


2018-08-22  Segher Boessenkool  

* combine.c (try_combine): Do not allow splitting a resulting PARALLEL
of two SETs into those two SETs, one to be placed at i2, if that SETs
destination is modified between i2 and i3.

---
 gcc/combine.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/gcc/combine.c b/gcc/combine.c
index 4ee20c8..2383a04 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -4064,7 +4064,10 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, 
rtx_insn *i0,
  other insns to combine, but the destination of that SET is still live.
 
  Also do this if we started with two insns and (at least) one of the
- resulting sets is a noop; this noop will be deleted later.  */
+ resulting sets is a noop; this noop will be deleted later.
+
+ Also do this if we started with two insns neither of which was a simple
+ move.  */
 
   else if (insn_code_number < 0 && asm_noperands (newpat) < 0
   && GET_CODE (newpat) == PARALLEL
@@ -4094,13 +4097,15 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, 
rtx_insn *i0,
 one which uses any regs/memory set in between i2 and i3 can't
 be first.  The PARALLEL might also have been pre-existing in i3,
 so we need to make sure that we won't wrongly hoist a SET to i2
-that would conflict with a death note present in there.  */
+that would conflict with a death note present in there, or would
+have its dest modified between i2 and i3.  */
   if (!modified_between_p (SET_SRC (set1), i2, i3)
  && !(REG_P (SET_DEST (set1))
   && find_reg_note (i2, REG_DEAD, SET_DEST (set1)))
  && !(GET_CODE (SET_DEST (set1)) == SUBREG
   && find_reg_note (i2, REG_DEAD,
 SUBREG_REG (SET_DEST (set1
+ && !modified_between_p (SET_DEST (set1), i2, i3)
  && (!HAVE_cc0 || !reg_referenced_p (cc0_rtx, set0))
  /* If I3 is a jump, ensure that set0 is a jump so that
 we do not create invalid RTL.  */
@@ -4116,6 +4121,7 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, 
rtx_insn *i0,
   && !(GET_CODE (SET_DEST (set0)) == SUBREG
&& find_reg_note (i2, REG_DEAD,
  SUBREG_REG (SET_DEST (set0
+  && !modified_between_p (SET_DEST (set0), i2, i3)
   && (!HAVE_cc0 || !reg_referenced_p (cc0_rtx, set1))
   /* If I3 is a jump, ensure that set1 is a jump so that
  we do not create invalid RTL.  */
-- 
1.8.3.1

Re: [PATCH, driver specs][2] Put -flto-partition= on the collect2 c/l

[Richard Biener]

On Wed, Aug 22, 2018 at 2:56 PM Iain Sandoe  wrote:
>
>
> > On 20 Aug 2018, at 11:01, Richard Biener  wrote:
> >
> > On Sat, Aug 18, 2018 at 9:00 PM Iain Sandoe  wrote:
> >>
>
> >> I plan on making Darwin default to fno-lto for link unless there’s an 
> >> “flto*” on the link line, since otherwise there’s a process launch for 
> >> every object on the c/l to do “nm”, which is quite heavy weight.  At 
> >> present, ISTM if the compiler is configured with —enable-lto, the link 
> >> line defaults to assuming that every object needs to be checked.
> >
> > I think we wanted to transparently handle LTO objects (similar to how
> > it works with a linker plugin).
>
> At present, we are not only calling nm for every object, but also dsymutil 
> sometimes unnecessarily - since the assumption on the “maybe_lto” path is 
> that an temporary file will be generated.
>
> as a headline - when I did the simplistic “default is to assume fno-lto” that 
> took 10% off the bootstrap time (with a stage#1 compiler without the 
> optimisation).
>
> - I have a patch under test as below + better fidelity of avoiding dsymutil 
> runs.
>
> > Ideally collect2 wouldn't use nm but simple-object to inspect files
> > (but that doesn't have symbol table query support
>
> Hrm.my WIP had grow symtab awareness to support debug-section copy on mach-o
> (and the ELF impl. looks like it understands both symtab and relocs)
> - so maybe that’s not as far away as we might think.
>
> > though looking for LTO specific sections would have been better in the
> > first place - I'd suggest .gnu.lto_.symtab).
>
> I’ve done this noting that the symtab seems to be the last emitted section - 
> is that why you chose it (for security, rather than just stopping as soon as 
> we see a .gnu.lto_. section?)

Ah, any .gnu.lto_.section would work as well I guess - I just picked
one that should be always
created.  .gnu.lto_.opts is another one.

> bootstrapped (Darwin and Linux) along with a third patch to make collect2 
> respond to -save-temps.
>
> comments?

OK.  Feel free to stop when recognizing any LTO section.  Btw, as you
include lto-section-names.h
you should probably use LTO_SECTION_NAME_PREFIX instead of hard-coding
.gnu.lto_.

I'm not sure if/how we should handle offload sections and if those
work at all without a
linker plugin currently?

Richard.

> thanks
> Iain
>
> gcc/
>
> * collect2.c (maybe_run_lto_and_relink): Don’t say we have a temp file
> unless we actually did some LTO.
> (has_lto_symtab, is_lto_object_file): New.
> (maybe_lto_object_file): Remove.
> (scan_prog_file): Use is_lto_object_file() instead of scanning the 
> output
> of nm.
>
> ---
>  gcc/collect2.c | 116 ++---
>  1 file changed, 53 insertions(+), 63 deletions(-)
>
> diff --git a/gcc/collect2.c b/gcc/collect2.c
> index 9ead5a6a1d..4b0f191ad3 100644
> --- a/gcc/collect2.c
> +++ b/gcc/collect2.c
> @@ -30,6 +30,8 @@ along with GCC; see the file COPYING3.  If not see
>  #include "tm.h"
>  #include "filenames.h"
>  #include "file-find.h"
> +#include "simple-object.h"
> +#include "lto-section-names.h"
>
>  /* TARGET_64BIT may be defined to use driver specific functionality. */
>  #undef TARGET_64BIT
> @@ -804,7 +806,9 @@ maybe_run_lto_and_relink (char **lto_ld_argv, char 
> **object_lst,
>/* Run the linker again, this time replacing the object files
>   optimized by the LTO with the temporary file generated by the LTO.  
> */
>fork_execute ("ld", out_lto_ld_argv, HAVE_GNU_LD && at_file_supplied);
> -  post_ld_pass (true);
> +  /* We assume that temp files were created, and therefore we need to 
> take
> + that into account (maybe run dsymutil).  */
> +  post_ld_pass (/*temp_file*/true);
>free (lto_ld_argv);
>
>maybe_unlink_list (lto_o_files);
> @@ -814,10 +818,11 @@ maybe_run_lto_and_relink (char **lto_ld_argv, char 
> **object_lst,
>/* Our caller is relying on us to do the link
>   even though there is no LTO back end work to be done.  */
>fork_execute ("ld", lto_ld_argv, HAVE_GNU_LD && at_file_supplied);
> -  post_ld_pass (false);
> +  /* No LTO objects were found, so no new temp file.  */
> +  post_ld_pass (/*temp_file*/false);
>  }
>else
> -post_ld_pass (true);
> +post_ld_pass (false); /* No LTO objects were found, no temp file.  */
>  }
>
>  /* Main program.  */
> @@ -1677,7 +1682,7 @@ main (int argc, char **argv)
> if (lto_mode != LTO_MODE_NONE)
>   maybe_run_lto_and_relink (ld1_argv, object_lst, object, false);
> else
> - post_ld_pass (false);
> + post_ld_pass (/*temp_file*/false);
>
> maybe_unlink (c_file);
> maybe_unlink (o_file);
> @@ -1749,7 +1754,7 @@ main (int argc, char **argv)
>  #ifdef COLLECT_EXPORT_LIST
>maybe_unlink (export_file);
>  #endif
> -  post_ld_pass (false);
> +  post_ld_pass (/*temp_file

Re: [PATCH, driver specs][3] Put -flto-partition= on the collect2 c/l

[Richard Biener]

On Wed, Aug 22, 2018 at 3:06 PM Iain Sandoe  wrote:
>
>
> > On 20 Aug 2018, at 11:01, Richard Biener  wrote:
> >
> > On Sat, Aug 18, 2018 at 9:00 PM Iain Sandoe  wrote:
> >>
> >> Hi
> >>
> >> While working on the Darwin LTO issues I noticed that collect2 looks for 
> >> "-flto-partition=none” in its command line option, but it doesn’t get 
> >> passed.
> >>
> >> So - is the attached patch the right idea, or should collect2 be looking 
> >> in the COLLECT_GCC_OPTIONS as the lto-wrapper does?
> >
> > Looking at COLLECT_GCC_OPTIONS is probably better.
>
> related;
> collect2 is currently “non-obvious” in that it doesn’t respond to -save-temps 
> and to get the tempories saved we have to invoke “-debug” which produces a 
> bunch of other output we probably don’t care about most times.

Yeah, I noticed this in the past as well (as well as buffering all
output without -debug).  But I
got used to specify -save-temps -Wl,-debug ...

>
> So .. perhaps we could move to
>  -save-temps (does the “usual thing”)
>  -v (does the “usual thing”)
>  -debug imples the two above + other informative output

Makes sense.

>
> (it would be kinda nice if there was some way to implement -### so that we 
> could see the collect subordinate pass structure)
>
> comments?

LGTM, aka OK.

Thanks,
Richard.

> thanks
> Iain
>
> gcc/
> * collect2.c (main): Parse the output file early so we can make nicer
> temp names.  Respond to “-save-temps” in the GCC OPTIONS.
> (maybe_unlink): Don’t print “[Leaving…”] for files we never created
> and don’t exist.
>
> ---
>  gcc/collect2.c | 57 +-
>  1 file changed, 43 insertions(+), 14 deletions(-)
>
> diff --git a/gcc/collect2.c b/gcc/collect2.c
> index 4b0f191ad3..b892e1a2aa 100644
> --- a/gcc/collect2.c
> +++ b/gcc/collect2.c
> @@ -205,8 +205,8 @@ bool helpflag;  /* true if --help */
>  static int shared_obj; /* true if -shared */
>  static int static_obj; /* true if -static */
>
> -static const char *c_file; /* .c for constructor/destructor 
> list.  */
> -static const char *o_file; /* .o for constructor/destructor 
> list.  */
> +static char *c_file;   /* .c for constructor/destructor list.  
> */
> +static char *o_file;   /* .o for constructor/destructor list.  
> */
>  #ifdef COLLECT_EXPORT_LIST
>  static const char *export_file;/* .x for AIX export 
> list.  */
>  #endif
> @@ -989,6 +989,13 @@ main (int argc, char **argv)
>
>save_temps = false;
>verbose = false;
> +
> +#ifndef DEFAULT_A_OUT_NAME
> +  output_file = "a.out";
> +#else
> +  output_file = DEFAULT_A_OUT_NAME;
> +#endif
> +
>/* Parse command line / environment for flags we want early.
>   This allows the debug flag to be set before functions like find_a_file()
>   are called. */
> @@ -1011,7 +1018,17 @@ main (int argc, char **argv)
>   selected_linker = USE_BFD_LD;
> else if (strcmp (argv[i], "-fuse-ld=gold") == 0)
>   selected_linker = USE_GOLD_LD;
> -
> +   else if (strncmp (argv[i], "-o", 2) == 0)
> + {
> +   /* Parse the output filename if it's given so that we can make
> +  meaningful temp filenames.  */
> +   if (argv[i][2] == '\0')
> + output_file = argv[i+1];
> +   else if (argv[i][2] == '=')
> + output_file = &argv[i][3];
> +   else
> + output_file = &argv[i][2];
> + }
>  #ifdef COLLECT_EXPORT_LIST
> /* These flags are position independent, although their order
>is important - subsequent flags override earlier ones. */
> @@ -1032,12 +1049,6 @@ main (int argc, char **argv)
>  #endif
>}
>
> -#ifndef DEFAULT_A_OUT_NAME
> -  output_file = "a.out";
> -#else
> -  output_file = DEFAULT_A_OUT_NAME;
> -#endif
> -
>  obstack_begin (&temporary_obstack, 0);
>  temporary_firstobj = (char *) obstack_alloc (&temporary_obstack, 0);
>
> @@ -1058,6 +1069,9 @@ main (int argc, char **argv)
>   no_partition = true;
> else if (strncmp (q, "-fno-lto", 8) == 0)
>   lto_mode = LTO_MODE_NONE;
> +   else if (strncmp (q, "-save-temps", 11) == 0)
> + /* FIXME: Honour =obj.  */
> + save_temps = true;
>  }
>  obstack_free (&temporary_obstack, temporary_firstobj);
>
> @@ -1217,8 +1231,22 @@ main (int argc, char **argv)
>*ld1++ = *ld2++ = ld_file_name;
>
>/* Make temp file names.  */
> -  c_file = make_temp_file (".c");
> -  o_file = make_temp_file (".o");
> +  if (save_temps)
> +{
> +  c_file = (char *) xmalloc (strlen (output_file)
> + + sizeof (".cdtor.c") + 1);
> +  strcpy (c_file, output_file);
> +  strcat (c_file, ".cdtor.c");
> +  o_file = (char *) xmalloc (strlen (output_file)
> + + sizeof (".cdtor.o") + 1);
> +  strcpy (o_file, output_file);

[PATCH][debug] Add -gdescriptive-dies

[Tom de Vries]

[ was: Re: [PATCH][debug] Add -gforce-named-dies ]

On 08/22/2018 11:46 AM, Tom de Vries wrote:
> On 08/22/2018 08:56 AM, Tom de Vries wrote:
>> This is an undocumented developer-only option, because using this option may
>> change behaviour of dwarf consumers, f.i., gdb shows the artificial 
>> variables:
>> ...
>> (gdb) info locals
>> a = 0x7fffda90 "\005"
>> D.4278 = 
>> ...
> I just found in the dwarf 5 spec the attribute DW_AT_description
> (present since version 3):
> ...
> 2.20 Entity Descriptions
> Some debugging information entries may describe entities in the program
> that are artificial, or which otherwise have a “name” that is not a
> valid identifier in the programming language.
> 
> This attribute provides a means for the producer to indicate the purpose
> or usage of the containing debugging infor
> 
> Generally, any debugging information entry that has, or may have, a
> DW_AT_name attribute, may also have a DW_AT_description attribute whose
> value is a null-terminated string providing a description of the entity.
> 
> It is expected that a debugger will display these descriptions as part
> of displaying other properties of an entity.
> ...
> 
> AFAICT, gdb currently does not explicitly handle this attribute, which I
> think means it's ignored.
> 
> It seems applicable to use DW_AT_description at least for the artificial
> decls.
> 
> Perhaps even for all cases that are added by the patch?
> 

I've chosen for this option. Using DW_AT_desciption instead of
DW_AT_name should minimize difference in gdb behaviour with and without
-gdescriptive-dies.

> I'll rewrite the patch.

OK for trunk?

Thanks,
- Tom
[debug] Add -gdescriptive-dies

This patch adds option -gdescriptive-dies.  It sets the DW_AT_description
attribute of dies that do not get a DW_AT_name attribute, to make it easier to
figure out what the die is describing.

The option exports the names of artificial variables:
...
 DIE0: DW_TAG_variable (0x7fa934dd54b0)
+  DW_AT_description: "D.1922"
   DW_AT_type: die -> 0 (0x7fa934dd0d70)
   DW_AT_artificial: 1

...
which can be traced back to gimple dumps:
...
  char a[0:D.1922] [value-expr: *a.0];
...

Furthermore, it adds names to external references:
...
 DIE0: DW_TAG_subprogram (0x7fa88b9650f0)
+DW_AT_description: "main"
 DW_AT_abstract_origin: die -> label: vla_1.c.6719312a + 29 (0x7fa88b965140)
...

Bootstrapped and reg-tested on x86_64, in combination with a patch that
switches the option on by default.

2018-08-15  Tom de Vries  

	* common.opt (gdescriptive-dies): Add option.
	* dwarf2out.c (add_name_and_src_coords_attributes): Add description
	attribute for artifical and nameless decls.
	(dwarf2out_register_external_die): Add description attribute to
	external reference die.
	(add_desc_attribute): New functions.
	(gen_subprogram_die): Add description attribute to
	DW_TAG_call_site_parameter.
	* doc/invoke.texi (@item Debugging Options): Add -gdescriptive-dies and
	-gno-descriptive-dies.
	(@item -gdescriptive-dies): Add.

---
 gcc/common.opt  |  4 
 gcc/doc/invoke.texi |  6 +-
 gcc/dwarf2out.c | 58 +
 3 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/gcc/common.opt b/gcc/common.opt
index ebc3ef43ce2..c2ba80c323f 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2976,6 +2976,10 @@ gstrict-dwarf
 Common Driver Report Var(dwarf_strict) Init(0)
 Don't emit DWARF additions beyond selected version.
 
+gdescriptive-dies
+Common Driver Report Var(flag_descriptive_dies) Init(0)
+Add description attribute to DWARF DIEs that have no name attribute.
+
 gtoggle
 Common Driver Report Var(flag_gtoggle)
 Toggle debug information generation.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 99849ec6467..5a9c5b49481 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -372,7 +372,7 @@ Objective-C and Objective-C++ Dialects}.
 -ginternal-reset-location-views  -gno-internal-reset-location-views @gol
 -ginline-points  -gno-inline-points @gol
 -gvms  -gxcoff  -gxcoff+  -gz@r{[}=@var{type}@r{]} @gol
--gsplit-dwarf @gol
+-gsplit-dwarf -gdescriptive-dies -gno-descriptive-dies @gol
 -fdebug-prefix-map=@var{old}=@var{new}  -fdebug-types-section @gol
 -fno-eliminate-unused-debug-types @gol
 -femit-struct-debug-baseonly  -femit-struct-debug-reduced @gol
@@ -7395,6 +7395,10 @@ the build system to avoid linking files with debug information.  To
 be useful, this option requires a debugger capable of reading @file{.dwo}
 files.
 
+@item -gdescriptive-dies
+@opindex gdescriptive-dies
+Add description attribute to DWARF DIEs that have no name attribute.
+
 @item -gpubnames
 @opindex gpubnames
 Generate DWARF @code{.debug_pubnames} and @code{.debug_pubtypes} sections.
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index fb71ff349fa..e1d4ea50695 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -3808,6 +3808,7 @@ static bool add_location_or_const_value_attribute (dw_die_ref, tree, bool);
 static bo

Re: [Patch, Fortran, F08] PR 86888: allocatable components of indirectly recursive type

[Paul Richard Thomas]

Hi Janus,


> the attached patch fixes the PR in the subject line in a rather
> straightforward fashion. Pointer components of indirectly recursive
> type are working already, as well as allocatable components of
> directly recursive type. It seems this case was simply forgotten.

That is correct. I was aware that it had been forgotten and is
somewhere far, far down on my TODO list. Thank you for dealing with
it.

OK for trunk.

Paul




-- 
"If you can't explain it simply, you don't understand it well enough"
- Albert Einstein

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Qing Zhao]

thanks.
now, I can repeat the failure.

Qing
> On Aug 21, 2018, at 7:25 PM, Paul Hua  wrote:
> 
> On Wed, Aug 22, 2018 at 2:15 AM Qing Zhao  > wrote:
>> 
>> 
>>> On Aug 21, 2018, at 8:07 AM, Paul Hua  wrote:
>>> 
>>> Hi, Qing,
>>> 
>>> The cfarm machine gcc23 can build mips64el successful, configure with
>>> "../configure --prefix=/opt/gcc-9 MISSING=texinfo MAKEINFO=missing
>>> --target=mips64el-linux-gnu --enable-languages=c,c++
>> 
>> I got the same failure message on gcc23 as gcc22, even with the above 
>> configure:
>> 
>> /usr/bin/ld: failed to merge target specific data of file 
>> /usr/lib32/libc.a(mremap.o)
>> /usr/bin/ld: /usr/lib32/libc.a(libc-lowlevellock.o): ABI is incompatible 
>> with that of the selected emulation
>> 
>> not sure what’s the problem?
>> 
> 
> I just build all-gcc and make check.
> 
> ./configure xxx
> make all-gcc -j 2
> make check-gcc RUNTESTFLAGS="dg.exp=strcmpopt_6.c"
> 
> It's enough to reproduce the regression.
> 
> Here is a mips port patch.
> 
> diff --git a/gcc/testsuite/gcc.dg/strcmpopt_6.c
> b/gcc/testsuite/gcc.dg/strcmpopt_6.c
> index 4c6de02824f..fa0ff9d1171 100644
> --- a/gcc/testsuite/gcc.dg/strcmpopt_6.c
> +++ b/gcc/testsuite/gcc.dg/strcmpopt_6.c
> @@ -33,4 +33,5 @@ int main (void)
> 
> }
> 
> -/* { dg-final { scan-assembler-times "memcmp" 2 } } */
> +/* { dg-final { scan-assembler-times "memcmp" 2 { target { !
> mips*-*-* } } } } */
> +/* { dg-final { scan-assembler-times "memcmp" 4 { target { mips*-*-* } } } } 
> */
> 
> Paul Hua

Re: [PATCH] Check the STRING_CSTs in varasm.c

[Bernd Edlinger]

Hi,


this is an updated version of my STRING_CSTs checking in varasm.c
patch.

It tries to answer the questions that were raised in th GO string literals
thread.

The answers are:
a) strings with TYPE_SIZE_UNIT == NULL do exist, but only for STRING_CSTs
in constructors of flexible array struct members.  Not for string literals.
b) In all cases where the STRING_CSTs do have a TYPE_SIZE_UNIT, the
DECL_SIZE_UNIT has the same value.
c) When STRING_CSTs do not have a TYPE_SIZE_UNIT, that is in the constructor
of a flexible array member.  In that case the TREE_STRING_LENGTH
determines the flexible array size.


It changes varasm's get_constant_size to return the TYPE_SIZE_UNIT of
a STRING_CST literal as it is, without increasing the storage size
to TREE_STRING_LENGTH.  I added an assertion to make sure that
all STRING_CSTs have a type size; size == 0 can happen for empty Ada
strings.

Furthermore it adds code to compare_constant to also compare the
STRING_CSTs TYPE_SIZE_UNIT if available.

So I want to remove that from get_constant_size in order to not change
the memory layout of GO and Ada strings, while still having them look
mostly like C string literals.

Furthermore I added one more consistency check to check_string_literal,
that makes sure that all STRING_CSTs that do have a TYPE_SIZE_UNIT,
the size matches the DECL_SIZE_UNIT.

Those newly discovered properties of string literals make the code in
c_strlen and friends a lot simpler.



Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.




On 08/17/18 15:53, Bernd Edlinger wrote:
> On 08/17/18 15:38, Richard Biener wrote:
>> On Fri, 17 Aug 2018, Bernd Edlinger wrote:
>>
>>> On 08/17/18 14:19, Richard Biener wrote:
 On Fri, 17 Aug 2018, Bernd Edlinger wrote:

> Richard Biener wrote:
>> +embedded @code{NUL} characters.  However, the
>> +@code{TREE_STRING_LENGTH} always includes a trailing @code{NUL} that
>> +is not part of the language string literal but appended by the front 
>> end.
>> +If the string shall not be @code{NUL}-terminated the @code{TREE_TYPE}
>> +is one character shorter than @code{TREE_STRING_LENGTH}.
>> +Excess caracters other than one trailing @code{NUL} character are not

 characters btw.

>>>
>>> thanks, updated.
>>>
 I read the above that the string literal for

 char x[2] = "1";

 is actually "1\0\0" - there's one NUL that is not part of the language
 string literal.  The second sentence then suggests that both \0
 are removed because 2 is less than 3?

>>>
>>> maybe 2 is a bad example, lets consider:
>>> char x[20] = "1";
>>>
>>> That is a string_cst with STRING_LENGTH = 2, content = "2\0\0"
>>> the array_type is used on both x, and the string_cst,
>>> I was assuming that both tree objects refer to the same type object.
>>> which is char[0..20-1]
>>
>> Oh, didn't realize we use char[2] for the STRING_CST.  Makes
>> my suggestion to use char[] instead not (very) much worse than the
>> existing practice then.
>>
>>> varasm assembles the bytes that are given by STRING_LENGTH
>>> and appends zeros as appropriate.
>>>
 As said, having this extra semantics of a STRING_CST tied to
 another tree node (its TREE_TYPE) looks ugly.

>> +permitted.
>>
>> I find this very confusing and oppose to that change.  Can we get
>> back to the drawing board please?  If we want an easy way to
>> see whether a string is "properly" terminated then maybe we can
>> simply use a flag that gets set by build_string?
>>
>
> What I mean with that is the case like
> char x[2] = "123456";
>
> which is build_string(7, "123456"), but with a type char[2],
> so varasm throws away "3456\0".

 I think varasm throws away chars not because of the type of
 the STRING_CST but because of the available storage in x.

>>>
>>> But at other places we look at the type of the string_cst, don't we?
>>> Shouldn't those be the same?
>>
>> I think most (all?) places look at TREE_TYPE (TREE_TYPE (string))
>> only.  I'm not aware of users of the array domain of the array type
>> of a string - but I'm far from knowing GCC inside-out ;)
>>
> 
> Yes, I know, that happens to me as well on the first day after my holidays ;)
> 
> I want to say that this is not okay, the excess precision
> should only be used to strip the nul termination, in cases
> where it is intended to be a assembled as a not zero terminated
> string.  But maybe the wording could be improved?

 ISTR we always assemble a NUL in .strings to get string merging
 working.

>>>
>>> String merging is not working when the string is not explicitly
>>> NUL terminated, my followup patch here tries to fix that:
>>>
>>> [PATCH] Handle not explicitly zero terminated strings in merge sections
>>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00481.html
>>
>> I'd have expected sth as simple a

[PATCH] Improve checks in c_strlen (PR 87053)

[Bernd Edlinger]

Hi!


This patch adds some more checks to c_getstr to fix PR middle-end/87053
wrong code bug.

Unfortunately this patch alone is not sufficient to fix the problem,
but also the patch for PR 86714 that hardens c_getstr is necessary
to prevent the wrong folding.


Bootstrapped and reg-tested on top of my PR 86711/86714 patch.
Is it OK for trunk?


Thanks
Bernd.


gcc:
2018-08-22  Bernd Edlinger  

	PR middle-end/87053
	* builtins.c (c_strlen): Improve range checks.

testsuite:
2018-08-22  Bernd Edlinger  

	PR middle-end/87053
	* gcc.c-torture/execute/pr87053.c: New test.

diff -Npur gcc/builtins.c gcc/builtins.c
--- gcc/builtins.c	2018-08-17 05:02:16.0 +0200
+++ gcc/builtins.c	2018-08-22 08:51:21.287960030 +0200
@@ -576,7 +576,7 @@ string_length (const void *ptr, unsigned
 tree
 c_strlen (tree src, int only_value, unsigned eltsize)
 {
-  gcc_assert (eltsize == 1 || eltsize == 2 || eltsize == 4);
+  gcc_checking_assert (eltsize == 1 || eltsize == 2 || eltsize == 4);
   STRIP_NOPS (src);
   if (TREE_CODE (src) == COND_EXPR
   && (only_value || !TREE_SIDE_EFFECTS (TREE_OPERAND (src, 0
@@ -665,10 +665,10 @@ c_strlen (tree src, int only_value, unsi
  a null character if we can represent it as a single HOST_WIDE_INT.  */
   if (byteoff == 0)
 eltoff = 0;
-  else if (! tree_fits_shwi_p (byteoff))
+  else if (! tree_fits_uhwi_p (byteoff) || tree_to_uhwi (byteoff) % eltsize)
 eltoff = -1;
   else
-eltoff = tree_to_shwi (byteoff) / eltsize;
+eltoff = tree_to_uhwi (byteoff) / eltsize;
 
   /* If the offset is known to be out of bounds, warn, and call strlen at
  runtime.  */
@@ -700,6 +700,10 @@ c_strlen (tree src, int only_value, unsi
   unsigned len = string_length (ptr + eltoff * eltsize, eltsize,
 strelts - eltoff);
 
+  /* Don't know what to return if there was no zero termination.  */
+  if (len > maxelts - eltoff)
+return NULL_TREE;
+
   return ssize_int (len);
 }
 
diff -Npur gcc/testsuite/gcc.c-torture/execute/pr87053.c gcc/testsuite/gcc.c-torture/execute/pr87053.c
--- gcc/testsuite/gcc.c-torture/execute/pr87053.c	1970-01-01 01:00:00.0 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr87053.c	2018-08-22 12:54:00.801019240 +0200
@@ -0,0 +1,17 @@
+/* PR middle-end/87053 */
+
+const union
+{ struct {
+char x[4];
+char y[4];
+  };
+  struct {
+char z[8];
+  };
+} u = {{"1234", "567"}};
+
+int main ()
+{
+  if (__builtin_strlen (u.z) != 7)
+__builtin_abort ();
+}

Re: [PATCH] Change AArch64 specific FMAX/FMIN tests into generic MAX_EXPR/MIN_EXPR tests

[Janne Blomqvist]

On Wed, Aug 22, 2018 at 4:16 PM, Szabolcs Nagy 
wrote:

> gfortran now always uses MAX_EXPR/MIN_EXPR for MAX/MIN intrinsics, so the
> AArch64 specific FMAX/FMIN tests are no longer valid.
>
> 2018-08-22  Szabolcs Nagy  
>
> * gfortran.dg/max_fmax_aarch64.f90: Rename to...
> * gfortran.dg/max_expr.f90: ...this.
> * gfortran.dg/min_fmin_aarch64.f90: Rename to...
> * gfortran.dg/min_expr.f90: ...this.
>

Ok, thanks!

-- 
Janne Blomqvist

Re: [PATCH] print full STRING_CST in Gimple dumps (PR 87052)

[Martin Sebor]

On 08/22/2018 06:48 AM, Richard Biener wrote:

On Wed, Aug 22, 2018 at 4:56 AM Martin Sebor  wrote:


In the discussion of the fallout from the enhancement for pr71625
it was pointed out that STRING_CSts in Gimple dumps extend only
to the first nul and don't include any subsequent characters,
and that this makes the dumps harder to read and might give rise
to the question whether the code is correct.

In the attached patch I enhance the pretty_print_string() function
to print the full contents of a STRING_CST, including any embedded
nuls to make the dumps clearer.  I got rid of the single digit
escapes like '\1' since they make the string look ambiguous.
If TREE_STRING_LENGTH (node) is guaranteed to be non-zero these
days the test for it being so may be redundant but I figured it's
better to be safe than sorry.

A further enhancement might be to also distinguish the type of
the STRING_CST.


And somehow indicate whether it is \0 terminated (just thinking of
the GIMPLE FE and how it parses string literals simply by relying
on libcpp).

Can you write a not \0 terminated string literal in C?


Yes: char a[2] = "12";

I briefly considered making the terminating nul visible when
there was one but there's at least one test that scans GIMPLE
for the expected string and it failed so I didn't pursue this
idea further.

The tests can certainly be changed to look for the new pattern
if we should decide to make a change here.  I'm not sure what
would be best.  Printing "12\x00" for an ordinary nul-terminated
literal looks like there might be an extra nul after the first
one.  Leaving it out doesn't distinguish it from the unterminated
array.  Using the braced-list notation (i.e.,  a = { '1', '2' }
for unterminated arrays doesn't capture the fact that it's
represented as STRING_CST).

I'm open to suggestions.

Martin

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Qing Zhao]

Hi, Rainer,

>From the comments you put into PR86519, for SPARC, looks like that only 32-bit 
>sparc has the problem.
sparcv9 does NOT have the same issue.

I was trying to find the string to represent 32-bit sparc target,   but haven’t 
found it.

my guess is:   sparc32*-*-*,  is this correct?


> On Aug 20, 2018, at 3:57 AM, Rainer Orth  
> wrote:
> 
> Hi Jeff,
> 
>>> 
>> Ugh.  :(  There's probably other targets that are going to have similar
>> properties.  I'm not really sure how to write a suitable assembler test
>> for this.
>> 
>> Maybe we just need a different search result for MIPS (and potentially
>> other targets).
> 
> we certainly do: I've reported a similar issue on sparc in PR
> testsuite/86519, but powerpc and s390x are equally affected.

do you mean that on powerpc and s390x, there is the same issue?

I tried on powerpc machines, seems no such issue. please advice.

thanks.

Qing

> 
>   Rainer
> 
> -- 
> -
> Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] Change AArch64 specific FMAX/FMIN tests into generic MAX_EXPR/MIN_EXPR tests

[Kyrill Tkachov]

Hi all,

On 22/08/18 15:57, Janne Blomqvist wrote:

On Wed, Aug 22, 2018 at 4:16 PM, Szabolcs Nagy 
wrote:

> gfortran now always uses MAX_EXPR/MIN_EXPR for MAX/MIN intrinsics, so the
> AArch64 specific FMAX/FMIN tests are no longer valid.
>
> 2018-08-22  Szabolcs Nagy 
>
> * gfortran.dg/max_fmax_aarch64.f90: Rename to...
> * gfortran.dg/max_expr.f90: ...this.
> * gfortran.dg/min_fmin_aarch64.f90: Rename to...
> * gfortran.dg/min_expr.f90: ...this.
>

Ok, thanks!



I've committed this on behalf of Szabolcs as r263778.

Thanks,
Kyrill


--
Janne Blomqvist

Re: Optimization in load_generic_interfaces()

[Thomas Koenig]

Hi Andrew,

[please also copy in gcc-patches for patches]


I'm continuing to look for optimizations to improve compile times for files
which USE large numbers of modules containing large numbers of symbols.

When the number of symbols becomes very large, find_symbol() becomes a slow-
point, because it can't use the structure of the balanced binary tree to
rapidly search the symtree, so just has to go through the whole tree until it
finds (or doesn't find) the symbol.

I don't see a simple way to improve the speed of this function, but there
seems to be a simple change in load_generic_interfaces() which gives
significant speed up:

Index: gcc/fortran/module.c
===
--- gcc/fortran/module.c(revision 263667)
+++ gcc/fortran/module.c(working copy)
@@ -4559,9 +4559,6 @@ load_generic_interfaces (void)
   /* Decide if we need to load this one or not.  */
   p = find_use_name_n (name, &i, false);
  
- st = find_symbol (gfc_current_ns->sym_root,

-   name, module_name, 1);
-
   if (!p || gfc_find_symbol (p, NULL, 0, &sym))
 {
   /* Skip the specific names for these cases.  */
@@ -4570,6 +4567,9 @@ load_generic_interfaces (void)
   continue;
 }
  
+ st = find_symbol (gfc_current_ns->sym_root,

+   name, module_name, 1);
+
   /* If the symbol exists already and is being USEd without being
  in an ONLY clause, do not load a new symtree(11.3.2).  */
   if (!only_flag && st)


This just delays the call to find_symbol() until after the first test of whether
the symbol needs to be loaded  - if that test fails then find_symbol() is never
called.

This has no significant effect on compile time for files which import small
numbers of symbols. But for cases where the number is large I find that the
compile time can be reduced by up to 40% in the cases I've tried.

The change passes all regression tests cleanly.


The patch is OK for trunk.

Thanks!

Regards

Thomas

Re: [PATCH v2][C][ADA] use function descriptors instead of trampolines in C

[Joseph Myers]

On Wed, 22 Aug 2018, Uecker, Martin wrote:

> Am Dienstag, den 21.08.2018, 21:34 + schrieb Joseph Myers:
> > On Tue, 21 Aug 2018, Uecker, Martin wrote:
> > 
> > > > I don't see why this is target-specific (if it is, the documentation 
> > > > for 
> > > > users in invoke.texi should explain what targets it works for and what 
> > > > it 
> > > > doesn't work for) anyway.  I'd expect it to be a target-independent 
> > > > feature with a target-independent test or tests.
> > > 
> > > My understanding is that this is a target-independent feature which
> > > has not yet been implemented for all targets. The existing
> > > documentation does not reflect this.
> > 
> > How does one tell which targets do or do not support it?
> 
> There is a target hook
> 
> TARGET_CUSTOM_FUNCTION_DESCRIPTORS
> 
> But I am not sure how to get this information to the testsuite.

Presumably through defining a check_* function in target-supports.exp that 
has a list of targets supporting the feature.  Together with 
cross-references in all the relevant places: the hook documentation in 
target.def (and thus the generated file tm.texi) should say that the list 
in target-supports.exp needs to be kept in sync if changing what targets 
implement the hook, and the list in target-supports.exp should similarly 
have a comment referencing the hook, and the user-level documentation in 
invoke.texi should list the relevant architectures, again with comments 
pointing in both directions between it and the other places needing 
updating when a change is made to the set of architectures supporting the 
feature.

Except: if the feature doesn't work on some targets, I'd expect an attempt 
to use -fno-trampolines on other targets to produce a sorry () message 
from the compiler, so it's immediately obvious to the user that the 
requested semantics are not being achieved.  And once you've got that 
sorry (), it should be something the check_* function can reliably test 
for (try compiling a trivial file with -fno-trampolines and see if it 
works, via check_no_compiler_messages*), so you don't need the list of 
targets in target-supports.exp in that case (but the documentation would 
need to say something about the option giving an error on targets not 
supporting it).

> there seems to be infrastructure to implement this. The information seems
> to come from a "target_info" structure (?) but I do not see where this
> is populated.

target_info comes from DejaGnu.  Sometimes a board file may set variables 
in target_info.  But for anything that is an architecture property of GCC, 
the board file should not need to set it; GCC should know the information 
itself.  The board file is more for describing board-specific information 
(e.g. memory available).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Rainer Orth]

Hi Qing,

> From the comments you put into PR86519, for SPARC, looks like that only
> 32-bit sparc has the problem.
> sparcv9 does NOT have the same issue.
>
> I was trying to find the string to represent 32-bit sparc target, but
> haven’t found it.
>
> my guess is:   sparc32*-*-*,  is this correct?

no, certainly not.  You need to use something like sparc*-*-* && ilp32
to catch the 32-bit multilib in both sparc-*-* and sparcv9-*-*
configurations.  This is similar to { i?86-*-* x86_64-*-* } && ilp32 on x86.

I'm still doubtful that enumerating target after target which all fail
the original test for unrelated reasons is the way to go, especially
given that there are others affected besides mips and sparc.

Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] Make strlen range computations more conservative

[Martin Sebor]

On 08/21/2018 10:05 PM, Bernd Edlinger wrote:

On 08/22/18 00:25, Jeff Law wrote:

On 08/21/2018 02:43 AM, Richard Biener wrote:

On Mon, 20 Aug 2018, Bernd Edlinger wrote:

[ snip. ]


Yes, I found some peanuts on my way.

For instance this fix for PR middle-end/86121 survives bootstrap on
it's own, and fixes one xfail.

Is it OK for trunk?


Yes, that's OK for trunk.

Agreed.  Seems like a nice independent bugfix and I don't think it
adversely affects anything else under current discussion.  In fact, not
folding here makes it easier to warn about incorrect code elsewhere.




Btw, I don't think we want sth like
flag_assume_zero_terminated_char_arrays or even make it default at
-Ofast.



Yes, I agree.  Is there a consensus about this?


Well, it's my own opinion of course.  Show me a benchmark that
improves with -fassume-zero-terminated-char-arrays.  Certainly
for security reasons it sounds a dangerous thing (and the documentation
needs a more thorough description of what it really means).

I certainly don't want to see a flag.  We've already got way too many;
adding another for marginal behavior just seems wrong.




If yes, I go ahead and remove that option again.

BTW: I needed this option in a few test cases, that insist in checking the
optimizer to eliminate stuff, based on the VRP info. (6 +/-1 or so).


Any example?


But we can as well remove those test cases.

Bernd, if there are specific tests that you want to see removed, we
should discuss them.



The test cases are:
gcc.dg/strlenopt-36.c


There are plenty of valid test cases in this test.  For example:

  extern char a7[7];
  if (strlen (a7) >= 7)   // fold to false
abort ();

Even if we wanted to accommodate common definitions the array
declarations could be changed to static and the tests would
be useful:

  static char a7[7];

There is no valid program where the if condition could be true.


gcc.dg/strlenopt-40.c


There are even more completely uncontroversial test cases here,
such as:

  if (strlen (i < 0 ? "123" : "4321") > 4)   // fold to false
abort ();


gcc.dg/strlenopt-45.c


Even more here.

  extern char c;
  if (strnlen (&c, 0) > 0)   // fold to false
abort ();
  if (strnlen (&c, 9) > 0)   // likewise
abort ();


gcc.dg/strlenopt-48.c
gcc.dg/strlenopt-51.c


All the test cases here include constant character arrays of
known length.  I see nothing controversial about any of them.



I see no way how to fix those, as they test the information flow
from the get_range_string to VRP info, which has to go away.

For the developing this patch it was fine to tweak the test cases
with the compiler flag, but I'd prefer to get rid of them.

There is one test that tests a warning, gcc.dg/pr83373.c.
I used the flag there, but could as well have simply xfailed that:


"Simply xfailing" tests for warnings would be inappropriate:
it would cause regressions and make the reporters unhappy.

Martin

Re: [PATCH] Improve checks in c_strlen (PR 87053)

[Martin Sebor]

On 08/22/2018 08:41 AM, Bernd Edlinger wrote:

Hi!


This patch adds some more checks to c_getstr to fix PR middle-end/87053
wrong code bug.

Unfortunately this patch alone is not sufficient to fix the problem,
but also the patch for PR 86714 that hardens c_getstr is necessary
to prevent the wrong folding.


Bootstrapped and reg-tested on top of my PR 86711/86714 patch.
Is it OK for trunk?


This case is also the subject of the patch I submitted back in
July for 86711/86714 and 86552.  With it, GCC avoid folding
the strlen call early and warns for the missing nul:

warning: ‘__builtin_strlen’ argument missing terminating nul 
[-Wstringop-overflow=]

   if (__builtin_strlen (u.z) != 7)
   ^~~~

The patch doesn't doesn't prevent all such strings from being
folded and it eventually lets fold_builtin_strlen() do its thing:

  /* To avoid warning multiple times about unterminated
 arrays only warn if its length has been determined
 and is being folded to a constant.  */
  if (nonstr)
warn_string_no_nul (loc, NULL_TREE, fndecl, nonstr);

  return fold_convert_loc (loc, type, len);

Handling this case is a matter of avoiding the folding here as
well and moving the warning later.

Since my patch is still in the review queue and does much more
than just prevent folding of non-nul terminated arrays it should
be reviewed first.

Martin

Re: Optimization in load_generic_interfaces()

[Andrew Benson]

Hi Thomas,

Thanks for the review and approval (and for reminding me to CC gcc-patches). 

I'm still working on getting my sourceware account for svn access set up, so I 
can't commit the patch myself yet. If you (or anyone else) wants to go ahead 
an commit it that would be good - otherwise I'll do so as soon as I have write 
access.

Thanks,
Andrew

On Wednesday, August 22, 2018 5:43:30 PM PDT Thomas Koenig wrote:
> Hi Andrew,
> 
> [please also copy in gcc-patches for patches]
> 
> > I'm continuing to look for optimizations to improve compile times for
> > files
> > which USE large numbers of modules containing large numbers of symbols.
> > 
> > When the number of symbols becomes very large, find_symbol() becomes a
> > slow- point, because it can't use the structure of the balanced binary
> > tree to rapidly search the symtree, so just has to go through the whole
> > tree until it finds (or doesn't find) the symbol.
> > 
> > I don't see a simple way to improve the speed of this function, but there
> > seems to be a simple change in load_generic_interfaces() which gives
> > significant speed up:
> > 
> > Index: gcc/fortran/module.c
> > ===
> > --- gcc/fortran/module.c(revision 263667)
> > +++ gcc/fortran/module.c(working copy)
> > @@ -4559,9 +4559,6 @@ load_generic_interfaces (void)
> > 
> >/* Decide if we need to load this one or not.  */
> >p = find_use_name_n (name, &i, false);
> > 
> > - st = find_symbol (gfc_current_ns->sym_root,
> > -   name, module_name, 1);
> > -
> > 
> >if (!p || gfc_find_symbol (p, NULL, 0, &sym))
> >
> >  {
> >  
> >/* Skip the specific names for these cases.  */
> > 
> > @@ -4570,6 +4567,9 @@ load_generic_interfaces (void)
> > 
> >continue;
> >  
> >  }
> > 
> > + st = find_symbol (gfc_current_ns->sym_root,
> > +   name, module_name, 1);
> > +
> > 
> >/* If the symbol exists already and is being USEd without being
> >
> >   in an ONLY clause, do not load a new symtree(11.3.2).  */
> >
> >if (!only_flag && st)
> > 
> > This just delays the call to find_symbol() until after the first test of
> > whether the symbol needs to be loaded  - if that test fails then
> > find_symbol() is never called.
> > 
> > This has no significant effect on compile time for files which import
> > small
> > numbers of symbols. But for cases where the number is large I find that
> > the
> > compile time can be reduced by up to 40% in the cases I've tried.
> > 
> > The change passes all regression tests cleanly.
> 
> The patch is OK for trunk.
> 
> Thanks!
> 
> Regards
> 
>   Thomas


-- 

* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Qing Zhao]

> On Aug 22, 2018, at 10:50 AM, Rainer Orth  
> wrote:
> 
> Hi Qing,
> 
>> From the comments you put into PR86519, for SPARC, looks like that only
>> 32-bit sparc has the problem.
>> sparcv9 does NOT have the same issue.
>> 
>> I was trying to find the string to represent 32-bit sparc target, but
>> haven’t found it.
>> 
>> my guess is:   sparc32*-*-*,  is this correct?
> 
> no, certainly not.  You need to use something like sparc*-*-* && ilp32
> to catch the 32-bit multilib in both sparc-*-* and sparcv9-*-*
> configurations.  This is similar to { i?86-*-* x86_64-*-* } && ilp32 on x86.

thanks for the info.

> 
> I'm still doubtful that enumerating target after target which all fail
> the original test for unrelated reasons is the way to go, especially
> given that there are others affected besides mips and sparc.

I am not sure, either.

however, given the available directives provided in testing suite, what’s the 
better solution
to this problem?

thanks.

Qing
> 
>   Rainer
> 
> -- 
> -
> Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [Patch, Fortran, F08] PR 86888: allocatable components of indirectly recursive type

[Janus Weil]

Am Mi., 22. Aug. 2018 um 15:44 Uhr schrieb Paul Richard Thomas
:
>
> > the attached patch fixes the PR in the subject line in a rather
> > straightforward fashion. Pointer components of indirectly recursive
> > type are working already, as well as allocatable components of
> > directly recursive type. It seems this case was simply forgotten.
>
> That is correct. I was aware that it had been forgotten and is
> somewhere far, far down on my TODO list. Thank you for dealing with
> it.

Thanks, Paul. Committed as r263782.

Cheers,
Janus

Re: [PATCH] Make strlen range computations more conservative

[Bernd Edlinger]

On 08/22/18 18:05, Martin Sebor wrote:
> On 08/21/2018 10:05 PM, Bernd Edlinger wrote:
>> On 08/22/18 00:25, Jeff Law wrote:
>>> On 08/21/2018 02:43 AM, Richard Biener wrote:
 On Mon, 20 Aug 2018, Bernd Edlinger wrote:
>>> [ snip. ]
>>>
> Yes, I found some peanuts on my way.
>
> For instance this fix for PR middle-end/86121 survives bootstrap on
> it's own, and fixes one xfail.
>
> Is it OK for trunk?

 Yes, that's OK for trunk.
>>> Agreed.  Seems like a nice independent bugfix and I don't think it
>>> adversely affects anything else under current discussion.  In fact, not
>>> folding here makes it easier to warn about incorrect code elsewhere.
>>>

>> Btw, I don't think we want sth like
>> flag_assume_zero_terminated_char_arrays or even make it default at
>> -Ofast.
>>
>
> Yes, I agree.  Is there a consensus about this?

 Well, it's my own opinion of course.  Show me a benchmark that
 improves with -fassume-zero-terminated-char-arrays.  Certainly
 for security reasons it sounds a dangerous thing (and the documentation
 needs a more thorough description of what it really means).
>>> I certainly don't want to see a flag.  We've already got way too many;
>>> adding another for marginal behavior just seems wrong.
>>>

> If yes, I go ahead and remove that option again.
>
> BTW: I needed this option in a few test cases, that insist in checking the
> optimizer to eliminate stuff, based on the VRP info. (6 +/-1 or so).

 Any example?

> But we can as well remove those test cases.
>>> Bernd, if there are specific tests that you want to see removed, we
>>> should discuss them.
>>>
>>
>> The test cases are:
>> gcc.dg/strlenopt-36.c
> 
> There are plenty of valid test cases in this test.  For example:
> 
>    extern char a7[7];
>    if (strlen (a7) >= 7)   // fold to false
>      abort ();
> 
> Even if we wanted to accommodate common definitions the array
> declarations could be changed to static and the tests would
> be useful:
> 
>    static char a7[7];
> 
> There is no valid program where the if condition could be true.
> 
>> gcc.dg/strlenopt-40.c
> 
> There are even more completely uncontroversial test cases here,
> such as:
> 
>    if (strlen (i < 0 ? "123" : "4321") > 4)   // fold to false
>      abort ();
> 

I see, the trouble is that the test case mixes valid cases with
cases that depend on type info in GIMPLE.

>> gcc.dg/strlenopt-45.c
> 
> Even more here.
> 
>    extern char c;
>    if (strnlen (&c, 0) > 0)   // fold to false
>      abort ();
>    if (strnlen (&c, 9) > 0)   // likewise
>      abort ();
> 
>> gcc.dg/strlenopt-48.c
>> gcc.dg/strlenopt-51.c
> 
> All the test cases here include constant character arrays of
> known length.  I see nothing controversial about any of them.
> 

Ah, sorry, a mistake in my changelog entry.  The strlenopt-51.c
test case does not need the flag.

I just changed this:
diff -Npur gcc/testsuite/gcc.dg/strlenopt-51.c 
gcc/testsuite/gcc.dg/strlenopt-51.c
--- gcc/testsuite/gcc.dg/strlenopt-51.c 2018-08-19 17:11:34.0 +0200
+++ gcc/testsuite/gcc.dg/strlenopt-51.c 2018-08-22 09:04:53.768302320 +0200
@@ -101,7 +101,7 @@ void test_keep_a9_9 (int i)
  {
  #undef T
  #define T(I)   \
-  KEEP (strlen (&a9_9[i][I][0]) > (1 + I) % 9);\
+  KEEP (strlen (&a9_9[i][I][0]) > (0 + I) % 9);\
KEEP (strlen (&a9_9[i][I][1]) > (1 + I) % 9);\
KEEP (strlen (&a9_9[i][I][2]) > (2 + I) % 9);\
KEEP (strlen (&a9_9[i][I][3]) > (3 + I) % 9);\
@@ -115,7 +115,7 @@ void test_keep_a9_9 (int i)
  }

  /* { dg-final { scan-tree-dump-times "strlen" 72 "gimple" } }
-   { dg-final { scan-tree-dump-times "strlen" 63 "optimized" } }
+   { dg-final { scan-tree-dump-times "strlen" 72 "optimized" } }

-   { dg-final { scan-tree-dump-times 
"call_made_in_true_branch_on_line_1\[0-9\]\[0-9\]\[0-9\]" 72 "optimized" } }
+   { dg-final { scan-tree-dump-times 
"call_made_in_true_branch_on_line_1\[0-9\]\[0-9\]\[0-9\]" 81 "optimized" } }
 { dg-final { scan-tree-dump-times 
"call_made_in_false_branch_on_line_1\[0-9\]\[0-9\]\[0-9\]" 81 "optimized" } } */

... which looks like the patch fixed something here,
although I cannot tell what.


>>
>> I see no way how to fix those, as they test the information flow
>> from the get_range_string to VRP info, which has to go away.
>>
>> For the developing this patch it was fine to tweak the test cases
>> with the compiler flag, but I'd prefer to get rid of them.
>>
>> There is one test that tests a warning, gcc.dg/pr83373.c.
>> I used the flag there, but could as well have simply xfailed that:
> 
> "Simply xfailing" tests for warnings would be inappropriate:
> it would cause regressions and make the reporters unhappy.
> 

Okay, but that is just one single test case.


Bernd.

[PATCH, rs6000] inline expansion of str[n]cmp using vec/vsx instructions

[Aaron Sawdey]

This patch teaches rs6000 inline expansion of strcmp/strncmp how to
generate vector/vsx code for power8/power9 processors. Compares 16
bytes and longer are generated using the vector code, which is
considerably faster than the gpr based code. 

Bootstrap/regtest passes on ppc64 (power8) and ppc64le (power8 and
power9). Ok for trunk?

Thanks!
Aaron


2018-08-22  Aaron Sawdey  

* config/rs6000/altivec.md (altivec_eq): Remove star.
* config/rs6000/rs6000-string.c (do_load_for_compare): Support
vector load modes.
(expand_strncmp_vec_sequence): New function.
(emit_final_str_compare_vec): New function.
(expand_strn_compare): Support for vector strncmp.
* config/rs6000/rs6000.opt (-mstring-compare-inline-limit): Change
length specification to bytes.
* config/rs6000/vsx.md (vsx_ld_elemrev_v16qi_internal): Remove star.
(vcmpnezb_p): New pattern.
* doc/invoke.texi (RS/6000 and PowerPC Options): Update documentation
for option -mstring-compare-inline-limit.

Index: gcc/config/rs6000/altivec.md
===
--- gcc/config/rs6000/altivec.md(revision 263753)
+++ gcc/config/rs6000/altivec.md(working copy)
@@ -603,7 +603,7 @@
   "vcmpbfp %0,%1,%2"
   [(set_attr "type" "veccmp")])
 
-(define_insn "*altivec_eq"
+(define_insn "altivec_eq"
   [(set (match_operand:VI2 0 "altivec_register_operand" "=v")
(eq:VI2 (match_operand:VI2 1 "altivec_register_operand" "v")
(match_operand:VI2 2 "altivec_register_operand" "v")))]
@@ -2304,7 +2304,7 @@
 
 ;; Compare vectors producing a vector result and a predicate, setting CR6 to
 ;; indicate a combined status
-(define_insn "*altivec_vcmpequ_p"
+(define_insn "altivec_vcmpequ_p"
   [(set (reg:CC CR6_REGNO)
(unspec:CC [(eq:CC (match_operand:VI2 1 "register_operand" "v")
   (match_operand:VI2 2 "register_operand" "v"))]
Index: gcc/config/rs6000/rs6000-string.c
===
--- gcc/config/rs6000/rs6000-string.c   (revision 263753)
+++ gcc/config/rs6000/rs6000-string.c   (working copy)
@@ -157,6 +157,29 @@
 {
   switch (GET_MODE (reg))
 {
+case E_V16QImode:
+  switch (mode) {
+  case E_V16QImode:
+   if (!BYTES_BIG_ENDIAN) 
+ if (TARGET_P9_VECTOR)
+   emit_insn (gen_vsx_ld_elemrev_v16qi_internal (reg, mem));
+ else
+   {
+ rtx reg_v2di = simplify_gen_subreg (V2DImode, reg, V16QImode, 0);
+ gcc_assert (MEM_P (mem));
+ rtx addr = XEXP (mem, 0);
+ rtx mem_v2di = gen_rtx_MEM (V2DImode, addr);
+ MEM_COPY_ATTRIBUTES (mem_v2di, mem);
+ set_mem_size (mem, GET_MODE_SIZE (V2DImode));
+ emit_insn (gen_vsx_ld_elemrev_v2di (reg_v2di, mem_v2di));
+   }
+   else
+ emit_insn (gen_vsx_movv2di_64bit (reg, mem));
+   break;
+  default:
+   gcc_unreachable ();
+  }
+  break;
 case E_DImode:
   switch (mode)
{
@@ -227,7 +250,22 @@
  gcc_unreachable ();
}
   break;
+
+case E_QImode:
+  switch (mode)
+   {
+   case E_QImode:
+ emit_move_insn (reg, mem);
+ break;
+   default:
+ debug_rtx(reg);
+ gcc_unreachable ();
+ break;
+   }
+  break;
+  
 default:
+  debug_rtx(reg);
   gcc_unreachable ();
   break;
 }
@@ -1878,6 +1916,175 @@
 
 }
 
+/* Generate the sequence of compares for strcmp/strncmp using vec/vsx 
+   instructions.
+
+   BYTES_TO_COMPARE is the number of bytes to be compared.
+   ORIG_SRC1 is the unmodified rtx for the first string.
+   ORIG_SRC2 is the unmodified rtx for the second string.
+   S1ADDR is the register to use for the base address of the first string.
+   S2ADDR is the register to use for the base address of the second string.
+   OFF_REG is the register to use for the string offset for loads.
+   S1DATA is the register for loading the first string.
+   S2DATA is the register for loading the second string.
+   VEC_RESULT is the rtx for the vector result indicating the byte difference.
+   EQUALITY_COMPARE_REST is a flag to indicate we need to make a cleanup call
+   to strcmp/strncmp if we have equality at the end of the inline comparison.
+   CLEANUP_LABEL is rtx for a label we generate if we need code to clean up
+   and generate the final comparison result.
+   FINAL_MOVE_LABEL is rtx for a label we can branch to when we can just 
+   set the final result.  */
+static void
+expand_strncmp_vec_sequence(unsigned HOST_WIDE_INT bytes_to_compare,
+   rtx orig_src1, rtx orig_src2,
+   rtx s1addr, rtx s2addr, rtx off_reg,
+   rtx s1data, rtx s2data,
+   rtx vec_result, bool equality_compare_rest,
+   rtx

Re: Optimization in load_generic_interfaces()

[Andrew Benson]

Hi Thomas,

I have my sourceware account active now, so I should be able to commit this 
myself. I'll do so right away.

Thanks,
Andrew

On Wednesday, August 22, 2018 5:43:30 PM PDT Thomas Koenig wrote:
> Hi Andrew,
> 
> [please also copy in gcc-patches for patches]
> 
> > I'm continuing to look for optimizations to improve compile times for
> > files
> > which USE large numbers of modules containing large numbers of symbols.
> > 
> > When the number of symbols becomes very large, find_symbol() becomes a
> > slow- point, because it can't use the structure of the balanced binary
> > tree to rapidly search the symtree, so just has to go through the whole
> > tree until it finds (or doesn't find) the symbol.
> > 
> > I don't see a simple way to improve the speed of this function, but there
> > seems to be a simple change in load_generic_interfaces() which gives
> > significant speed up:
> > 
> > Index: gcc/fortran/module.c
> > ===
> > --- gcc/fortran/module.c(revision 263667)
> > +++ gcc/fortran/module.c(working copy)
> > @@ -4559,9 +4559,6 @@ load_generic_interfaces (void)
> > 
> >/* Decide if we need to load this one or not.  */
> >p = find_use_name_n (name, &i, false);
> > 
> > - st = find_symbol (gfc_current_ns->sym_root,
> > -   name, module_name, 1);
> > -
> > 
> >if (!p || gfc_find_symbol (p, NULL, 0, &sym))
> >
> >  {
> >  
> >/* Skip the specific names for these cases.  */
> > 
> > @@ -4570,6 +4567,9 @@ load_generic_interfaces (void)
> > 
> >continue;
> >  
> >  }
> > 
> > + st = find_symbol (gfc_current_ns->sym_root,
> > +   name, module_name, 1);
> > +
> > 
> >/* If the symbol exists already and is being USEd without being
> >
> >   in an ONLY clause, do not load a new symtree(11.3.2).  */
> >
> >if (!only_flag && st)
> > 
> > This just delays the call to find_symbol() until after the first test of
> > whether the symbol needs to be loaded  - if that test fails then
> > find_symbol() is never called.
> > 
> > This has no significant effect on compile time for files which import
> > small
> > numbers of symbols. But for cases where the number is large I find that
> > the
> > compile time can be reduced by up to 40% in the cases I've tried.
> > 
> > The change passes all regression tests cleanly.
> 
> The patch is OK for trunk.
> 
> Thanks!
> 
> Regards
> 
>   Thomas


-- 

* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus

Re: Optimization in load_generic_interfaces()

[Andrew Benson]

Committed as r263784.

Thanks,
Andrew

On Wednesday, August 22, 2018 5:43:30 PM PDT Thomas Koenig wrote:
> Hi Andrew,
> 
> [please also copy in gcc-patches for patches]
> 
> > I'm continuing to look for optimizations to improve compile times for
> > files
> > which USE large numbers of modules containing large numbers of symbols.
> > 
> > When the number of symbols becomes very large, find_symbol() becomes a
> > slow- point, because it can't use the structure of the balanced binary
> > tree to rapidly search the symtree, so just has to go through the whole
> > tree until it finds (or doesn't find) the symbol.
> > 
> > I don't see a simple way to improve the speed of this function, but there
> > seems to be a simple change in load_generic_interfaces() which gives
> > significant speed up:
> > 
> > Index: gcc/fortran/module.c
> > ===
> > --- gcc/fortran/module.c(revision 263667)
> > +++ gcc/fortran/module.c(working copy)
> > @@ -4559,9 +4559,6 @@ load_generic_interfaces (void)
> > 
> >/* Decide if we need to load this one or not.  */
> >p = find_use_name_n (name, &i, false);
> > 
> > - st = find_symbol (gfc_current_ns->sym_root,
> > -   name, module_name, 1);
> > -
> > 
> >if (!p || gfc_find_symbol (p, NULL, 0, &sym))
> >
> >  {
> >  
> >/* Skip the specific names for these cases.  */
> > 
> > @@ -4570,6 +4567,9 @@ load_generic_interfaces (void)
> > 
> >continue;
> >  
> >  }
> > 
> > + st = find_symbol (gfc_current_ns->sym_root,
> > +   name, module_name, 1);
> > +
> > 
> >/* If the symbol exists already and is being USEd without being
> >
> >   in an ONLY clause, do not load a new symtree(11.3.2).  */
> >
> >if (!only_flag && st)
> > 
> > This just delays the call to find_symbol() until after the first test of
> > whether the symbol needs to be loaded  - if that test fails then
> > find_symbol() is never called.
> > 
> > This has no significant effect on compile time for files which import
> > small
> > numbers of symbols. But for cases where the number is large I find that
> > the
> > compile time can be reduced by up to 40% in the cases I've tried.
> > 
> > The change passes all regression tests cleanly.
> 
> The patch is OK for trunk.
> 
> Thanks!
> 
> Regards
> 
>   Thomas


-- 

* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus

[libiberty patch] Fix PGO bootstrap

[Nathan Sidwell]

Martin discovered that my recent pex-unix change broke profiled 
bootstrap, and also confirmed this patch fixes the problem.


The change is to break the two live ranges of 'bad_fn' into separate 
scoped declarations each entirely within the child or the parent.


While there, rename a local variable to avoid shadowing a parameter.

ok?

nathan
--
Nathan Sidwell
2018-08-22  Nathan Sidwell  
	Martin Liska  

	* pex-unix.c (pex_unix_exec_child): Duplicate bad_fn into local
	scopes to avoid potential clobber.

Index: pex-unix.c
===
--- pex-unix.c	(revision 263766)
+++ pex-unix.c	(working copy)
@@ -582,8 +582,6 @@ pex_unix_exec_child (struct pex_obj *obj
  issues.   */
   char **save_environ = environ;
 
-  const char *bad_fn = NULL;
-
   for (retries = 0; retries < 4; ++retries)
 {
   pid = vfork ();
@@ -602,62 +600,64 @@ pex_unix_exec_child (struct pex_obj *obj
 
 case 0:
   /* Child process.  */
-  if (!bad_fn && in != STDIN_FILE_NO)
-	{
-	  if (dup2 (in, STDIN_FILE_NO) < 0)
-	bad_fn = "dup2";
-	  else if (close (in) < 0)
-	bad_fn = "close";
-	}
-  if (!bad_fn && out != STDOUT_FILE_NO)
-	{
-	  if (dup2 (out, STDOUT_FILE_NO) < 0)
-	bad_fn = "dup2";
-	  else if (close (out) < 0)
-	bad_fn = "close";
-	}
-  if (!bad_fn && errdes != STDERR_FILE_NO)
-	{
-	  if (dup2 (errdes, STDERR_FILE_NO) < 0)
-	bad_fn = "dup2";
-	  else if (close (errdes) < 0)
-	bad_fn = "close";
-	}
-  if (!bad_fn && toclose >= 0)
-	{
-	  if (close (toclose) < 0)
-	bad_fn = "close";
-	}
-  if (!bad_fn && (flags & PEX_STDERR_TO_STDOUT) != 0)
-	{
-	  if (dup2 (STDOUT_FILE_NO, STDERR_FILE_NO) < 0)
-	bad_fn = "dup2";
-	}
-  if (!bad_fn)
-	{
-	  if (env)
-	/* NOTE: In a standard vfork implementation this clobbers
-	   the parent's copy of environ "too" (in reality there's
-	   only one copy).  This is ok as we restore it below.  */
-	environ = (char**) env;
-	  if ((flags & PEX_SEARCH) != 0)
-	{
-	  execvp (executable, to_ptr32 (argv));
-	  bad_fn = "execvp";
-	}
-	  else
-	{
-	  execv (executable, to_ptr32 (argv));
-	  bad_fn = "execv";
-	}
-	}
-
-  /* Something failed, report an error.  We don't use stdio
-	 routines, because we might be here due to a vfork call.  */
   {
-	ssize_t retval = 0;
-	int err = errno;
+	const char *bad_fn = NULL;
+
+	if (!bad_fn && in != STDIN_FILE_NO)
+	  {
+	if (dup2 (in, STDIN_FILE_NO) < 0)
+	  bad_fn = "dup2";
+	else if (close (in) < 0)
+	  bad_fn = "close";
+	  }
+	if (!bad_fn && out != STDOUT_FILE_NO)
+	  {
+	if (dup2 (out, STDOUT_FILE_NO) < 0)
+	  bad_fn = "dup2";
+	else if (close (out) < 0)
+	  bad_fn = "close";
+	  }
+	if (!bad_fn && errdes != STDERR_FILE_NO)
+	  {
+	if (dup2 (errdes, STDERR_FILE_NO) < 0)
+	  bad_fn = "dup2";
+	else if (close (errdes) < 0)
+	  bad_fn = "close";
+	  }
+	if (!bad_fn && toclose >= 0)
+	  {
+	if (close (toclose) < 0)
+	  bad_fn = "close";
+	  }
+	if (!bad_fn && (flags & PEX_STDERR_TO_STDOUT) != 0)
+	  {
+	if (dup2 (STDOUT_FILE_NO, STDERR_FILE_NO) < 0)
+	  bad_fn = "dup2";
+	  }
+	if (!bad_fn)
+	  {
+	if (env)
+	  /* NOTE: In a standard vfork implementation this clobbers
+		 the parent's copy of environ "too" (in reality there's
+		 only one copy).  This is ok as we restore it below.  */
+	  environ = (char**) env;
+	if ((flags & PEX_SEARCH) != 0)
+	  {
+		execvp (executable, to_ptr32 (argv));
+		bad_fn = "execvp";
+	  }
+	else
+	  {
+		execv (executable, to_ptr32 (argv));
+		bad_fn = "execv";
+	  }
+	  }
 
+	/* Something failed, report an error.  We don't use stdio
+	   routines, because we might be here due to a vfork call.  */
+	ssize_t retval = 0;
+	int eno = errno;
+	
 #define writeerr(s) (retval |= write (STDERR_FILE_NO, s, strlen (s)))
 	writeerr (obj->pname);
 	writeerr (": error trying to exec '");
@@ -665,7 +665,7 @@ pex_unix_exec_child (struct pex_obj *obj
 	writeerr ("': ");
 	writeerr (bad_fn);
 	writeerr (": ");
-	writeerr (xstrerror (err));
+	writeerr (xstrerror (eno));
 	writeerr ("\n");
 #undef writeerr
 
@@ -677,30 +677,33 @@ pex_unix_exec_child (struct pex_obj *obj
 
 default:
   /* Parent process.  */
+  {
+	const char *bad_fn = NULL;
+	
+	/* Restore environ.  Note that the parent either doesn't run
+	   until the child execs/exits (standard vfork behaviour), or
+	   if it does run then vfork is behaving more like fork.  In
+	   either case we needn't worry about clobbering the child's
+	   copy of environ.  */
+	environ = save_environ;
 
-  /* Restore environ.
-	 Note that the parent either doesn't run until the child execs/exits
-	 (standard vfork behaviour), or if it does run then vfork is behaving
-	 more like fork.  In either case we needn't worry about clobbering
-	 the child's copy of environ.  */
-  environ = save_environ;
-
-  if (!ba

[libiberty patch] Fix PGO bootstrap

[Nathan Sidwell]

Martin discovered that my recent pex-unix change broke profiled 
bootstrap, and also confirmed this patch fixes the problem.


The change is to break the two live ranges of 'bad_fn' into separate 
scoped declarations each entirely within the child or the parent.


While there, rename a local variable to avoid shadowing a parameter.

ok?

nathan
--
Nathan Sidwell
2018-08-22  Nathan Sidwell  
	Martin Liska  

	* pex-unix.c (pex_unix_exec_child): Duplicate bad_fn into local
	scopes to avoid potential clobber.

Index: pex-unix.c
===
--- pex-unix.c	(revision 263766)
+++ pex-unix.c	(working copy)
@@ -582,8 +582,6 @@ pex_unix_exec_child (struct pex_obj *obj
  issues.   */
   char **save_environ = environ;
 
-  const char *bad_fn = NULL;
-
   for (retries = 0; retries < 4; ++retries)
 {
   pid = vfork ();
@@ -602,62 +600,64 @@ pex_unix_exec_child (struct pex_obj *obj
 
 case 0:
   /* Child process.  */
-  if (!bad_fn && in != STDIN_FILE_NO)
-	{
-	  if (dup2 (in, STDIN_FILE_NO) < 0)
-	bad_fn = "dup2";
-	  else if (close (in) < 0)
-	bad_fn = "close";
-	}
-  if (!bad_fn && out != STDOUT_FILE_NO)
-	{
-	  if (dup2 (out, STDOUT_FILE_NO) < 0)
-	bad_fn = "dup2";
-	  else if (close (out) < 0)
-	bad_fn = "close";
-	}
-  if (!bad_fn && errdes != STDERR_FILE_NO)
-	{
-	  if (dup2 (errdes, STDERR_FILE_NO) < 0)
-	bad_fn = "dup2";
-	  else if (close (errdes) < 0)
-	bad_fn = "close";
-	}
-  if (!bad_fn && toclose >= 0)
-	{
-	  if (close (toclose) < 0)
-	bad_fn = "close";
-	}
-  if (!bad_fn && (flags & PEX_STDERR_TO_STDOUT) != 0)
-	{
-	  if (dup2 (STDOUT_FILE_NO, STDERR_FILE_NO) < 0)
-	bad_fn = "dup2";
-	}
-  if (!bad_fn)
-	{
-	  if (env)
-	/* NOTE: In a standard vfork implementation this clobbers
-	   the parent's copy of environ "too" (in reality there's
-	   only one copy).  This is ok as we restore it below.  */
-	environ = (char**) env;
-	  if ((flags & PEX_SEARCH) != 0)
-	{
-	  execvp (executable, to_ptr32 (argv));
-	  bad_fn = "execvp";
-	}
-	  else
-	{
-	  execv (executable, to_ptr32 (argv));
-	  bad_fn = "execv";
-	}
-	}
-
-  /* Something failed, report an error.  We don't use stdio
-	 routines, because we might be here due to a vfork call.  */
   {
-	ssize_t retval = 0;
-	int err = errno;
+	const char *bad_fn = NULL;
+
+	if (!bad_fn && in != STDIN_FILE_NO)
+	  {
+	if (dup2 (in, STDIN_FILE_NO) < 0)
+	  bad_fn = "dup2";
+	else if (close (in) < 0)
+	  bad_fn = "close";
+	  }
+	if (!bad_fn && out != STDOUT_FILE_NO)
+	  {
+	if (dup2 (out, STDOUT_FILE_NO) < 0)
+	  bad_fn = "dup2";
+	else if (close (out) < 0)
+	  bad_fn = "close";
+	  }
+	if (!bad_fn && errdes != STDERR_FILE_NO)
+	  {
+	if (dup2 (errdes, STDERR_FILE_NO) < 0)
+	  bad_fn = "dup2";
+	else if (close (errdes) < 0)
+	  bad_fn = "close";
+	  }
+	if (!bad_fn && toclose >= 0)
+	  {
+	if (close (toclose) < 0)
+	  bad_fn = "close";
+	  }
+	if (!bad_fn && (flags & PEX_STDERR_TO_STDOUT) != 0)
+	  {
+	if (dup2 (STDOUT_FILE_NO, STDERR_FILE_NO) < 0)
+	  bad_fn = "dup2";
+	  }
+	if (!bad_fn)
+	  {
+	if (env)
+	  /* NOTE: In a standard vfork implementation this clobbers
+		 the parent's copy of environ "too" (in reality there's
+		 only one copy).  This is ok as we restore it below.  */
+	  environ = (char**) env;
+	if ((flags & PEX_SEARCH) != 0)
+	  {
+		execvp (executable, to_ptr32 (argv));
+		bad_fn = "execvp";
+	  }
+	else
+	  {
+		execv (executable, to_ptr32 (argv));
+		bad_fn = "execv";
+	  }
+	  }
 
+	/* Something failed, report an error.  We don't use stdio
+	   routines, because we might be here due to a vfork call.  */
+	ssize_t retval = 0;
+	int eno = errno;
+	
 #define writeerr(s) (retval |= write (STDERR_FILE_NO, s, strlen (s)))
 	writeerr (obj->pname);
 	writeerr (": error trying to exec '");
@@ -665,7 +665,7 @@ pex_unix_exec_child (struct pex_obj *obj
 	writeerr ("': ");
 	writeerr (bad_fn);
 	writeerr (": ");
-	writeerr (xstrerror (err));
+	writeerr (xstrerror (eno));
 	writeerr ("\n");
 #undef writeerr
 
@@ -677,30 +677,33 @@ pex_unix_exec_child (struct pex_obj *obj
 
 default:
   /* Parent process.  */
+  {
+	const char *bad_fn = NULL;
+	
+	/* Restore environ.  Note that the parent either doesn't run
+	   until the child execs/exits (standard vfork behaviour), or
+	   if it does run then vfork is behaving more like fork.  In
+	   either case we needn't worry about clobbering the child's
+	   copy of environ.  */
+	environ = save_environ;
 
-  /* Restore environ.
-	 Note that the parent either doesn't run until the child execs/exits
-	 (standard vfork behaviour), or if it does run then vfork is behaving
-	 more like fork.  In either case we needn't worry about clobbering
-	 the child's copy of environ.  */
-  environ = save_environ;
-
-  if (!ba

Re: Async I/O patch with compilation fix

[David Edelsohn]

Thomas,

This patch broke bootstrap on AIX again.  This is completely unacceptable.

In file included from
*/nasfarm/edelsohn/src/src/libgfortran/runtime/error.c:28*:

*/nasfarm/edelsohn/src/src/libgfortran/io/async.h:333:3:* *error: *unknown
type name '*__gthread_cond_t*'

333 |   *__gthread_cond_t* signal;

|   *^~~~*

make[1]: *** [Makefile:2594: error.lo] Error 1

- David

On Tue, Aug 21, 2018 at 3:42 PM Thomas Koenig  wrote:

> Hi everybody,
>
> Nicolas has committed the patch as r263750.
>
> PR 87048 now traces the armeb regression, which is
> assumed to resurface now.  Let's really try and fix
> that one before 9.0 :-)
>
> Regards
>
> Thomas
>

[PATCH] DWARF: Call set_indirect_string on DW_MACINFO_start_file

[H.J. Lu]

Since -gsplit-dwarf -g3 will output filename as indirect string, call
set_indirect_string on DW_MACINFO_start_file for -gsplit-dwarf -g3.

OK for trunk?

H.J.
--
PR debug/79342
* dwarf2out.c (save_macinfo_strings): Call set_indirect_string
on DW_MACINFO_start_file for -gsplit-dwarf -g3
---
 gcc/dwarf2out.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index edf1ac35896..6ae0a4d66b4 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -28280,6 +28280,12 @@ save_macinfo_strings (void)
 && (debug_str_section->common.flags & SECTION_MERGE) != 0)
   set_indirect_string (find_AT_string (ref->info));
 break;
+ case DW_MACINFO_start_file:
+   /* -gsplit-dwarf -g3 will also output filename as indirect
+  string.  */
+   if (!dwarf_split_debug_info)
+ break;
+   /* Fall through. */
  case DW_MACRO_define_strp:
  case DW_MACRO_undef_strp:
 set_indirect_string (find_AT_string (ref->info));
-- 
2.17.1

Re: GCC fixinclude help

[Bruce Korb]

On 08/15/2018 02:34 PM, Albert Chin-A-Young wrote:
Don't suppose you have time to help with: 
https://gcc.gnu.org/ml/gcc/2018-08/msg00102.html

David Edelsohn is correct:

6. Now that you have the right things happening, synchronize the

$(srcdir)/tests/base directory with the $(builddir)/tests/res

directory. The output of "make check" will be some diffs that

should give you some hints about what to do.

PING^1: [PATCH] Set start_location to 0 if we ran out of line map space

[H.J. Lu]

PING.


-- Forwarded message --
From: H.J. Lu 
Date: Wed, Aug 15, 2018 at 4:33 AM
Subject: [PATCH] Set start_location to 0 if we ran out of line map space
To: gcc-patches@gcc.gnu.org


With profiledbootstrap and --with-build-config=bootstrap-lto, linemap_add
may create a macro map when we run out of line map space.  This patch
changes start_location to UNKNOWN_LOCATION (0) in this case.

Tested with profiledbootstrap and --with-build-config=bootstrap-lto on
Linux/x86-64.

PR bootstrap/86872
* line-map.c (pure_location_p): Return true if linemap_lookup
returns NULL.
(linemap_add): Set start_location to 0 if we run out of line map
space.
---
 libcpp/line-map.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index 555cd129a9c..cafe42273eb 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -304,6 +304,8 @@ pure_location_p (line_maps *set, source_location loc)
 return false;

   const line_map *map = linemap_lookup (set, loc);
+  if (map == NULL)
+return true;
   const line_map_ordinary *ordmap = linemap_check_ordinary (map);

   if (loc & ((1U << ordmap->m_range_bits) - 1))
@@ -492,6 +494,11 @@ linemap_add (struct line_maps *set, enum lc_reason reason,
 }

   linemap_assert (reason != LC_ENTER_MACRO);
+
+  if (start_location >= LINE_MAP_MAX_LOCATION)
+/* We ran out of line map space.   */
+start_location = 0;
+
   line_map_ordinary *map
 = linemap_check_ordinary (new_linemap (set, start_location));
   map->reason = reason;
--
2.17.1



-- 
H.J.

[PATCH] Signed zero for {max,min}val intrinsics

[Janne Blomqvist]

The Fortran standard specifies (e.g. F2018 7.4.3.2) that intrinsic
procedures shall treat positive and negative real zero as equivalent,
unless it is explicitly specified otherwise.  For {max,min}val there
is no such explicit mention.  Thus, remove code to handle signed
zeros.

Regtested on x86_64-pc-linux-gnu, Ok for trunk?

2018-08-22  Janne Blomqvist  

* trans-intrinsic.c (gfc_conv_intrinsic_minmaxval): Delete
HONOR_SIGNED_ZEROS checks.
---
 gcc/fortran/trans-intrinsic.c | 26 ++
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/gcc/fortran/trans-intrinsic.c b/gcc/fortran/trans-intrinsic.c
index 387cf80b921..b2cea93742a 100644
--- a/gcc/fortran/trans-intrinsic.c
+++ b/gcc/fortran/trans-intrinsic.c
@@ -5511,22 +5511,10 @@ gfc_conv_intrinsic_minmaxval (gfc_se * se, gfc_expr * 
expr, enum tree_code op)
 {
   /* MIN_EXPR/MAX_EXPR has unspecified behavior with NaNs or
 signed zeros.  */
-  if (HONOR_SIGNED_ZEROS (DECL_MODE (limit)))
-   {
- tmp = fold_build2_loc (input_location, op, logical_type_node,
-arrayse.expr, limit);
- ifbody = build2_v (MODIFY_EXPR, limit, arrayse.expr);
- tmp = build3_v (COND_EXPR, tmp, ifbody,
- build_empty_stmt (input_location));
- gfc_add_expr_to_block (&block2, tmp);
-   }
-  else
-   {
- tmp = fold_build2_loc (input_location,
-op == GT_EXPR ? MAX_EXPR : MIN_EXPR,
-type, arrayse.expr, limit);
- gfc_add_modify (&block2, limit, tmp);
-   }
+  tmp = fold_build2_loc (input_location,
+op == GT_EXPR ? MAX_EXPR : MIN_EXPR,
+type, arrayse.expr, limit);
+  gfc_add_modify (&block2, limit, tmp);
 }
 
   if (fast)
@@ -5535,8 +5523,7 @@ gfc_conv_intrinsic_minmaxval (gfc_se * se, gfc_expr * 
expr, enum tree_code op)
 
   /* MIN_EXPR/MAX_EXPR has unspecified behavior with NaNs or
 signed zeros.  */
-  if (HONOR_NANS (DECL_MODE (limit))
- || HONOR_SIGNED_ZEROS (DECL_MODE (limit)))
+  if (HONOR_NANS (DECL_MODE (limit)))
{
  tmp = fold_build2_loc (input_location, op, logical_type_node,
 arrayse.expr, limit);
@@ -5598,8 +5585,7 @@ gfc_conv_intrinsic_minmaxval (gfc_se * se, gfc_expr * 
expr, enum tree_code op)
 
   /* MIN_EXPR/MAX_EXPR has unspecified behavior with NaNs or
 signed zeros.  */
-  if (HONOR_NANS (DECL_MODE (limit))
- || HONOR_SIGNED_ZEROS (DECL_MODE (limit)))
+  if (HONOR_NANS (DECL_MODE (limit)))
{
  tmp = fold_build2_loc (input_location, op, logical_type_node,
 arrayse.expr, limit);
-- 
2.17.1

Allow target to emit LTO early debug to a separate LTO file.

[Iain Sandoe]

Hi Tom, Richi,

This is something I was experimenting with to try and solve platform problems 
with early debug.

Not a “finished patch” (I’ve removed the Darwin back-end parts) but would like 
your comments on the central idea.

This is to switch to the alternate LTO file (this process already exists for 
the actual LTO output) before the early debug is started and switch back to the 
regular output file after.  Therefore both the LTO early debug and the LTO 
streamed data end up in a separate file (this can be concatenated as we do now, 
guaranteeing that it appears after any referenced symbols, or could be handled 
in “some other way” if that was a useful solution).

Now the second part of this delays the output of the .file directives until the 
“regular” output of the asm (it could be that this could be simplified now 
there there’s a start/end function pair).

The idea is that the main output text is identical with/without the early debug 
(and, in fact, it’s broken without this change - since the .file directives 
would end up in the separate LTO stream).

thoughts?
Iain


diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index ec490d75bd..1a7db6c353 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -2777,11 +2777,14 @@ symbol_table::finalize_compilation_unit (void)
   FOR_EACH_FUNCTION_WITH_GIMPLE_BODY (cnode)
(*debug_hooks->early_global_decl) (cnode->decl);
 
+ targetm.asm_out.lto_start ();
   /* Clean up anything that needs cleaning up after initial debug
 generation.  */
   debuginfo_early_start ();
   (*debug_hooks->early_finish) (main_input_filename);
+
   debuginfo_early_stop ();
+  targetm.asm_out.lto_end ();
 }
 
   /* Finally drive the pass manager.  */


diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 74a5926524..5f166b7f42 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -3600,6 +3600,9 @@ static GTY(()) unsigned int poc_label_num;
 /* The last file entry emitted by maybe_emit_file().  */
 static GTY(()) struct dwarf_file_data * last_emitted_file;
 
+/* Don't emit the .file directives for early debug.  */
+static bool delay_emit_file = false;
+
 /* Number of internal labels generated by gen_internal_sym().  */
 static GTY(()) int label_num;
 
@@ -26939,7 +26942,7 @@ lookup_filename (const char *file_name)
 static int
 maybe_emit_file (struct dwarf_file_data * fd)
 {
-  if (! fd->emitted_number)
+  if (! fd->emitted_number && ! delay_emit_file)
 {
   if (last_emitted_file)
fd->emitted_number = last_emitted_file->emitted_number + 1;
@@ -31866,6 +31869,7 @@ dwarf2out_early_finish (const char *filename)
   set_early_dwarf s;
   char dl_section_ref[MAX_ARTIFICIAL_LABEL_BYTES];
 
+  delay_emit_file = true;
   /* PCH might result in DW_AT_producer string being restored from the
  header compilation, so always fill it with empty string initially
  and overwrite only here.  */
@@ -31917,6 +31921,7 @@ dwarf2out_early_finish (const char *filename)
  fprintf (dump_file, "LTO EARLY DWARF for %s\n", filename);
  print_die (comp_unit_die (), dump_file);
}
+  delay_emit_file = false;
   return;
 }
 
@@ -32006,7 +32011,11 @@ dwarf2out_early_finish (const char *filename)
 copy_lto_debug_sections operation of the simple object support in
 libiberty is not implemented for them yet.  */
   || TARGET_PECOFF || TARGET_COFF)
-return;
+  || TARGET_PECOFF)
+{
+  delay_emit_file = false;
+  return;
+}
 
   /* Now as we are going to output for LTO initialize sections and labels
  to the LTO variants.  We don't need a random-seed postfix as other
@@ -32128,6 +32137,8 @@ dwarf2out_early_finish (const char *filename)
output_indirect_string> (form);
 }
 
+  delay_emit_file = false;
+
   /* Switch back to the text section.  */
   switch_to_section (text_section);
 }
-- 
2.17.1

Re: [PATCH] Check the STRING_CSTs in varasm.c

[Martin Sebor]

On 08/22/2018 08:27 AM, Bernd Edlinger wrote:

Hi,


this is an updated version of my STRING_CSTs checking in varasm.c
patch.

It tries to answer the questions that were raised in th GO string literals
thread.

The answers are:
a) strings with TYPE_SIZE_UNIT == NULL do exist, but only for STRING_CSTs
in constructors of flexible array struct members.  Not for string literals.
b) In all cases where the STRING_CSTs do have a TYPE_SIZE_UNIT, the
DECL_SIZE_UNIT has the same value.
c) When STRING_CSTs do not have a TYPE_SIZE_UNIT, that is in the constructor
of a flexible array member.  In that case the TREE_STRING_LENGTH
determines the flexible array size.


It changes varasm's get_constant_size to return the TYPE_SIZE_UNIT of
a STRING_CST literal as it is, without increasing the storage size
to TREE_STRING_LENGTH.  I added an assertion to make sure that
all STRING_CSTs have a type size; size == 0 can happen for empty Ada
strings.

Furthermore it adds code to compare_constant to also compare the
STRING_CSTs TYPE_SIZE_UNIT if available.

So I want to remove that from get_constant_size in order to not change
the memory layout of GO and Ada strings, while still having them look
mostly like C string literals.

Furthermore I added one more consistency check to check_string_literal,
that makes sure that all STRING_CSTs that do have a TYPE_SIZE_UNIT,
the size matches the DECL_SIZE_UNIT.

Those newly discovered properties of string literals make the code in
c_strlen and friends a lot simpler.



Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?



@@ -1162,9 +1162,13 @@ These nodes represent string-constants.

 returns the length of the string, as an @code{int}.  The

 @code{TREE_STRING_POINTER} is a @code{char*} containing the string

 itself.  The string may not be @code{NUL}-terminated, and it may contain

-embedded @code{NUL} characters.  Therefore, the

-@code{TREE_STRING_LENGTH} includes the trailing @code{NUL} if it is

-present.

+embedded @code{NUL} characters.  However, the

+@code{TREE_STRING_LENGTH} always includes a trailing @code{NUL} that

+is not part of the language string literal but appended by the front end.

+If the string shall not be @code{NUL}-terminated the @code{TREE_TYPE}


If the string is not NUL-terminated (not shall not be -- that
makes it sound like it must not be nul-terminated).

+is one character shorter than @code{TREE_STRING_LENGTH}.


Presumably the phrase "the @code{TREE_TYPE} is shorter" means
that the type of the string is an array whose domain is
[0, TREE_STRING_LENGTH - 1], or something like that.  It would
help to make this clear in a less informal way, especially if
not all STRING_CST types have a domain (sounds like some don't
if they have a null TYPE_SIZE_UNIT).

+Excess characters other than one trailing @code{NUL} character are not

+permitted.



Does this mean that they can be counted on not to exist
because the front ends make sure they don't and the middle
end doesn't create them, or that should not be created but
that they might still exist?

You also mentioned a lot of detail in your answers above that
isn't captured here.  Can that be added?  E.g., the part about
TYPE_SIZE_UNIT of a STRING_CST being allowed to be null seems
especially important, as does the bit about flexible array
members.

Martin

Make safe_iterator inline friends

[François Dumont]

Now that _Safe_iterator has been revisited I would like to cleanup its 
operators to make them globals and inline friends as much as possible.


This patch transform operator-(const _Safe_iterator<>&, difference_type) 
and operator+(const _Safe_iterator<>&, difference_type) into global 
namespace operators.


Otherwise it just make all operators inline friend.

Only operator== and != remains outside _Safe_iterator because all my 
attempts to make them inline friends failed. I understand that an inline 
friend within a base class is not a very clean design.


Compiler error was:

/home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/debug/safe_iterator.h:459: 
error: redefinition of 'bool __gnu_debug::operator==(const _Self&, const 
_OtherSelf&)'
/home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/debug/safe_iterator.h:452: 
note: 'bool __gnu_debug::operator==(const _Self&, const _Self&)' 
previously declared here
/home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/debug/safe_iterator.h:473: 
error: redefinition of 'bool __gnu_debug::operator!=(const _Self&, const 
_OtherSelf&)'
/home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/debug/safe_iterator.h:466: 
note: 'bool __gnu_debug::operator!=(const _Self&, const _Self&)' 
previously declared here


I don't know if it is a compiler issue but keeping those operators that 
should apply for any category of safe iterator at namespace level is ok. 
There are not much operators within __gnu_debug and users normally don't 
explicitely use this namespace.


I added even more checks in testsuite_containers.h especially using 
std::rel_ops to make sure we have no ambiguity.


Tested under Linux x86_64 debug mode.

Ok to commit ?

François

diff --git a/libstdc++-v3/include/debug/safe_iterator.h b/libstdc++-v3/include/debug/safe_iterator.h
index 86211b9..b573197 100644
--- a/libstdc++-v3/include/debug/safe_iterator.h
+++ b/libstdc++-v3/include/debug/safe_iterator.h
@@ -36,6 +36,28 @@
 #include 
 #include 
 
+#define _GLIBCXX_DEBUG_VERIFY_OPERANDS(_Lhs, _Rhs, _BadMsgId, _DiffMsgId) \
+  _GLIBCXX_DEBUG_VERIFY(!_Lhs._M_singular() && !_Rhs._M_singular(),	\
+			_M_message(_BadMsgId)\
+			._M_iterator(_Lhs, #_Lhs)			\
+			._M_iterator(_Rhs, #_Rhs));			\
+  _GLIBCXX_DEBUG_VERIFY(_Lhs._M_can_compare(_Rhs),			\
+			_M_message(_DiffMsgId)\
+			._M_iterator(_Lhs, #_Lhs)			\
+			._M_iterator(_Rhs, #_Rhs))
+
+#define _GLIBCXX_DEBUG_VERIFY_CMP_OPERANDS(_Lhs, _Rhs)			\
+  _GLIBCXX_DEBUG_VERIFY_OPERANDS(_Lhs, _Rhs, __msg_iter_compare_bad,	\
+ __msg_compare_different)
+
+#define _GLIBCXX_DEBUG_VERIFY_ORDER_OPERANDS(_Lhs, _Rhs)		\
+  _GLIBCXX_DEBUG_VERIFY_OPERANDS(_Lhs, _Rhs, __msg_iter_order_bad,	\
+ __msg_order_different)
+
+#define _GLIBCXX_DEBUG_VERIFY_DIST_OPERANDS(_Lhs, _Rhs)			\
+  _GLIBCXX_DEBUG_VERIFY_OPERANDS(_Lhs, _Rhs, __msg_distance_bad,	\
+ __msg_distance_different)
+
 namespace __gnu_debug
 {
   /** Helper struct to deal with sequence offering a before_begin
@@ -374,7 +396,7 @@ namespace __gnu_debug
 
   // Can we advance the iterator @p __n steps (@p __n may be negative)
   bool
-  _M_can_advance(const difference_type& __n) const;
+  _M_can_advance(difference_type __n) const;
 
   // Is the iterator range [*this, __rhs) valid?
   bool
@@ -574,6 +596,11 @@ namespace __gnu_debug
 			 std::bidirectional_iterator_tag> _Safe_base;
   typedef typename _Safe_base::_OtherIterator _OtherIterator;
 
+  typedef _Safe_iterator<_Iterator, _Sequence,
+			 std::random_access_iterator_tag> _Self;
+  typedef _Safe_iterator<_OtherIterator, _Sequence,
+			 std::random_access_iterator_tag> _OtherSelf;
+
   typedef typename _Safe_base::_Attach_single _Attach_single;
 
   _Safe_iterator(_Iterator __i, _Safe_sequence_base* __seq, _Attach_single)
@@ -706,7 +733,7 @@ namespace __gnu_debug
 
   // -- Random access iterator requirements --
   reference
-  operator[](const difference_type& __n) const _GLIBCXX_NOEXCEPT
+  operator[](difference_type __n) const _GLIBCXX_NOEXCEPT
   {
 	_GLIBCXX_DEBUG_VERIFY(this->_M_can_advance(__n)
 			  && this->_M_can_advance(__n + 1),
@@ -716,7 +743,7 @@ namespace __gnu_debug
   }
 
   _Safe_iterator&
-  operator+=(const difference_type& __n) _GLIBCXX_NOEXCEPT
+  operator+=(difference_type __n) _GLIBCXX_NOEXCEPT
   {
 	_GLIBCXX_DEBUG_VERIFY(this->_M_can_advance(__n),
 			  _M_message(__msg_advance_oob)
@@ -726,17 +753,8 @@ namespace __gnu_debug
 	return *this;
   }
 
-  _Safe_iterator
-  operator+(const difference_type& __n) const _GLIBCXX_NOEXCEPT
-  {
-	_GLIBCXX_DEBUG_VERIFY(this->_M_can_advance(__n),
-			  _M_message(__msg_advance_oob)
-			  ._M_iterator(*this)._M_integer(__n));
-	return _Safe_iterator(this->base() + __n, this->_M_sequence);
-  }
-
   _Safe_iterator&
-  operator-=(const difference_type& __n) _GLIBCXX_NOEXCEPT
+  operator-=(dif

Re: Async I/O patch with compilation fix

[Thomas Koenig]

Hi David,


This patch broke bootstrap on AIX again.  This is completely unacceptable.


Again, sorry for the breakage.

I faced quite some challenges trying to get bootstrap to
work on gcc119. Despite quite a few e-mails (plus hints in a PR)
that I received, none of the hints for bootstrap I got actually got
it to work. I finally gave up after four or five different failures,
and the patch was committed because every test
that _could_ be run did show no failure.

Had we received instructions that work for bootstrapping on AIX,
we would have tested the patch there.

If it were possible to add instructions that do work for AIX
bootstrapping to the compile farm wiki, that would be great.

If you (or somebody else who has the requisite AIX fu) could test
patches that are known to be difficult, that would also be
great.

As long as we have no other solution, it is probably best to #ifdef out
AIX any additional pthread-related functionality for libgfortran that
might be coming along. That can always be integrated later, if somebody
can re-implement POSIX condition variables for libgfortran from what
AIX provides.

Let's talk about how to proceed at the GNU cauldron, over a beer.
Both Nicolas and I will be there.

In the meantime, I have committed the following patch (r263788) as
obvious after regression-testing on Linux both with ASYNC_IO set
to 1 and to 0.  Let me know how that works out.

2018-08-22  Thomas Koenig  

* async.h: Set ASYNC_IO to zero if _AIX is defined.
(struct adv_cond): If ASYNC_IO is zero, the struct has no members.
(async_unit): If ASYNC_IO is zero, remove unneeded members.

2018-08-22  Thomas Koenig  

* gfortran.texi: Mention that asynchronous I/O does
not work on systems which lack condition variables, such
as AIX.

Regards

Thomas
Index: gcc/fortran/gfortran.texi
===
--- gcc/fortran/gfortran.texi	(Revision 263752)
+++ gcc/fortran/gfortran.texi	(Arbeitskopie)
@@ -1509,7 +1509,8 @@
 
 Asynchronous I/O is supported if the program is linked against the
 POSIX thread library. If that is not the case, all I/O is performed
-as synchronous.
+as synchronous. On systems which do not support pthread condition
+variables, such as AIX, I/O is also performed as synchronous.
 
 On some systems, such as Darwin or Solaris, the POSIX thread library
 is always linked in, so asynchronous I/O is always performed. On other
Index: libgfortran/io/async.h
===
--- libgfortran/io/async.h	(Revision 263752)
+++ libgfortran/io/async.h	(Arbeitskopie)
@@ -29,7 +29,7 @@
__gthread_cond_t and __gthread_equal / __gthread_self.  Check
this.  */
 
-#if defined(__GTHREAD_HAS_COND) && defined(__GTHREADS_CXX0X)
+#if defined(__GTHREAD_HAS_COND) && defined(__GTHREADS_CXX0X) && !defined(_AIX)
 #define ASYNC_IO 1
 #else
 #define ASYNC_IO 0
@@ -328,21 +328,18 @@
 
 struct adv_cond
 {
+#if ASYNC_IO
   int pending;
   __gthread_mutex_t lock;
   __gthread_cond_t signal;
+#endif
 };
 
 typedef struct async_unit
 {
+  pthread_mutex_t io_lock;   /* Lock for doing actual I/O. */
   pthread_mutex_t lock;  /* Lock for manipulating the queue structure.  */
-  pthread_mutex_t io_lock;   /* Lock for doing actual I/O. */
-  struct adv_cond work;
-  struct adv_cond emptysignal;
-  struct st_parameter_dt *pdt;
-  pthread_t thread;
-  struct transfer_queue *head;
-  struct transfer_queue *tail;
+  bool empty;
   struct
   {
 int waiting;
@@ -351,7 +348,13 @@
 struct adv_cond done;
   } id;
 
-  bool empty;
+#if ASYNC_IO
+  struct adv_cond work;
+  struct adv_cond emptysignal;
+  struct st_parameter_dt *pdt;
+  pthread_t thread;
+  struct transfer_queue *head;
+  struct transfer_queue *tail;
 
   struct {
 const char *message;
@@ -361,7 +364,7 @@
 int family;
 bool fatal_error;
   } error;
-
+#endif
 } async_unit;
 
 void init_async_unit (gfc_unit *);

Re: Make safe_iterator inline friends

[Jonathan Wakely]

On 22/08/18 23:08 +0200, François Dumont wrote:
Only operator== and != remains outside _Safe_iterator because all my 
attempts to make them inline friends failed. I understand that an 
inline friend within a base class is not a very clean design.


Compiler error was:

/home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/debug/safe_iterator.h:459: 
error: redefinition of 'bool __gnu_debug::operator==(const _Self&, 
const _OtherSelf&)'
/home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/debug/safe_iterator.h:452: 
note: 'bool __gnu_debug::operator==(const _Self&, const _Self&)' 
previously declared here
/home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/debug/safe_iterator.h:473: 
error: redefinition of 'bool __gnu_debug::operator!=(const _Self&, 
const _OtherSelf&)'
/home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/debug/safe_iterator.h:466: 
note: 'bool __gnu_debug::operator!=(const _Self&, const _Self&)' 
previously declared here


I don't know if it is a compiler issue


I don't think so. The error seems clear: when _Self and _OtherSelf are
the same type the friend declarations are the same function.

Re: [PATCH][Middle-end]patch for fixing PR 86519

[Jeff Law]

On 08/22/2018 11:05 AM, Qing Zhao wrote:
> 
>> On Aug 22, 2018, at 10:50 AM, Rainer Orth  
>> wrote:
>>
>> Hi Qing,
>>
>>> From the comments you put into PR86519, for SPARC, looks like that only
>>> 32-bit sparc has the problem.
>>> sparcv9 does NOT have the same issue.
>>>
>>> I was trying to find the string to represent 32-bit sparc target, but
>>> haven’t found it.
>>>
>>> my guess is:   sparc32*-*-*,  is this correct?
>>
>> no, certainly not.  You need to use something like sparc*-*-* && ilp32
>> to catch the 32-bit multilib in both sparc-*-* and sparcv9-*-*
>> configurations.  This is similar to { i?86-*-* x86_64-*-* } && ilp32 on x86.
> 
> thanks for the info.
> 
>>
>> I'm still doubtful that enumerating target after target which all fail
>> the original test for unrelated reasons is the way to go, especially
>> given that there are others affected besides mips and sparc.
> 
> I am not sure, either.
> 
> however, given the available directives provided in testing suite, what’s the 
> better solution
> to this problem?
We could move the test into the i386 target specific test directory.
It'll still get good coverage that way and it'll be naturally restricted
to a target where we don't have to worry about the function name we're
scanning for showing up in undesirable contexts.

Another approach might be to scan the RTL dumps.  But if there were a
target that didn't have direct jumps and requires a hi+lo_sum style
sequence to load a symbolic constant it would fail.

jeff

Re: patch to bug #86829

[Jeff Law]

On 08/22/2018 06:02 AM, Richard Biener wrote:
> On Tue, Aug 21, 2018 at 11:27 PM Jeff Law  wrote:
>>
>> On 08/21/2018 02:08 PM, Giuliano Augusto Faulin Belinassi wrote:
 Just as an example, compare the results for
 x = 0x1.fp1023
>>>
>>> Thank you for your answer and the counterexample. :-)
>>>
 If we had useful range info on floats we might conditionalize such
 transforms appropriately.  Or we can enable it on floats and do
 the sqrt (x*x + 1) in double.
>>>
>>> I think I managed to find a bound were the transformation can be done
>>> without overflow harm, however I don't know about rounding problems,
>>> however
>>>
>>> Suppose we are handling double precision floats for now. The function
>>> x/sqrt(1 + x*x) approaches 1 when x is big enough. How big must be x
>>> for the function be 1?
>>>
>>> Since sqrt(1 + x*x) > x when x > 1, then we must find a value to x
>>> that x/sqrt(1 + x*x) < eps, where eps is the biggest double smaller
>>> than 1. Such eps must be around 1 - 2^-53 in ieee double because the
>>> mantissa has 52 bits. Solving for x yields that x must be somewhat
>>> bigger than 6.7e7, so let's take 1e8. Therefore if abs(x) > 1e8, it is
>>> enough to return copysign(1, x). Notice that this arguments is also
>>> valid for x = +-inf (if target supports that) because sin(atan(+-inf))
>>> = +-1, and it can be extended to other floating point formats.The
>>> following test code illustrates my point:
>>> https://pastebin.com/M4G4neLQ
>>>
>>> This might still be faster than calculating sin(atan(x)) explicitly.
>>>
>>> Please let me know if this is unfeasible. :-)
>> The problem is our VRP implementation doesn't handle any floating point
>> types at this time.   If we had range information for FP types, then
>> this kind of analysis is precisely what we'd need to do the
>> transformation regardless of -ffast-math.
> 
> I think his idea was to emit a runtime test?  You'd have to use a
> COND_EXPR and evaluate both arms at the same time because
> match.pd doesn't allow you to create control flow.
> 
> Note the rounding issue is also real given for large x you strip
> away lower mantissa bits when computing x*x.
Ah, a runtime test.  That'd be sufficient.  The cost when we can't do
the transformation is relatively small, but the gains when we can are huge.

Jeff

Re: [libiberty patch] Fix PGO bootstrap

[Ian Lance Taylor via gcc-patches]

On Wed, Aug 22, 2018 at 11:34 AM, Nathan Sidwell  wrote:
> Martin discovered that my recent pex-unix change broke profiled bootstrap,
> and also confirmed this patch fixes the problem.
>
> The change is to break the two live ranges of 'bad_fn' into separate scoped
> declarations each entirely within the child or the parent.
>
> While there, rename a local variable to avoid shadowing a parameter.
>
> ok?

This is OK.

Thanks.

Ian

[PATCH] PR libstdc++/78448 limit vector::max_size and deque::max_size

[Jonathan Wakely]

The container requirements imply that max_size() can't exceed the
maximum value of the container's difference_type. Enforce this for
std::vector and std::deque, and add checks to ensure the container
doesn't grow larger than that.

PR libstdc++/78448
* include/bits/deque.tcc (deque::_M_range_initialize): Use
_S_check_init_len to check size.
(deque::_M_push_back_aux, deque::_M_push_front_aux): Throw length
error if size would exceed max_size().
* include/bits/stl_deque.h (_Deque_base::size_type): Remove typedef.
(_Deque_base(_Deque_base&&, const allocator_type&, size_t)): Use
size_t instead of size_type.
(deq(size_type, const allocator_type&)
(deq(size_type, const value_type&, const allocator_type&)
(deque::_M_initialize_dispatch): Use _S_check_init_len to check size.
(deque::max_size): Call _S_max_size.
(deque::_S_check_init_len, deque::_S_max_size): New functions.
* include/bits/stl_vector.h (vector(size_type, const allocator_type&))
(vector(size_type, const value_type&, const allocator_type&))
(vector::_M_initialize_dispatch, vector::_M_range_initialize): Use
_S_check_init_len to check size.
(vector::max_size): Call _S_max_size.
(vector::_M_check_len): Prevent max from being expanded as a
function-like macro.
(vector::_S_check_init_len, vector::_S_max_size): New functions.
* include/bits/vector.tcc (vector::_M_assign_aux): Use
_S_check_init_len to check size.
* testsuite/23_containers/deque/capacity/max_size.cc: New test.
* testsuite/23_containers/vector/capacity/max_size.cc: New test.

Tested x86_64-linux, committed to trunk.


commit 76ee81e30063c70022f200514cc3b74933f2bf88
Author: Jonathan Wakely 
Date:   Wed Aug 22 22:29:53 2018 +0100

PR libstdc++/78448 limit vector::max_size and deque::max_size

The container requirements imply that max_size() can't exceed the
maximum value of the container's difference_type. Enforce this for
std::vector and std::deque, and add checks to ensure the container
doesn't grow larger than that.

PR libstdc++/78448
* include/bits/deque.tcc (deque::_M_range_initialize): Use
_S_check_init_len to check size.
(deque::_M_push_back_aux, deque::_M_push_front_aux): Throw length
error if size would exceed max_size().
* include/bits/stl_deque.h (_Deque_base::size_type): Remove typedef.
(_Deque_base(_Deque_base&&, const allocator_type&, size_t)): Use
size_t instead of size_type.
(deq(size_type, const allocator_type&)
(deq(size_type, const value_type&, const allocator_type&)
(deque::_M_initialize_dispatch): Use _S_check_init_len to check 
size.
(deque::max_size): Call _S_max_size.
(deque::_S_check_init_len, deque::_S_max_size): New functions.
* include/bits/stl_vector.h (vector(size_type, const 
allocator_type&))
(vector(size_type, const value_type&, const allocator_type&))
(vector::_M_initialize_dispatch, vector::_M_range_initialize): Use
_S_check_init_len to check size.
(vector::max_size): Call _S_max_size.
(vector::_M_check_len): Prevent max from being expanded as a
function-like macro.
(vector::_S_check_init_len, vector::_S_max_size): New functions.
* include/bits/vector.tcc (vector::_M_assign_aux): Use
_S_check_init_len to check size.
* testsuite/23_containers/deque/capacity/max_size.cc: New test.
* testsuite/23_containers/vector/capacity/max_size.cc: New test.

diff --git a/libstdc++-v3/include/bits/deque.tcc 
b/libstdc++-v3/include/bits/deque.tcc
index 8724a19504b..a22948a9753 100644
--- a/libstdc++-v3/include/bits/deque.tcc
+++ b/libstdc++-v3/include/bits/deque.tcc
@@ -443,7 +443,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
   std::forward_iterator_tag)
   {
 const size_type __n = std::distance(__first, __last);
-this->_M_initialize_map(__n);
+this->_M_initialize_map(_S_check_init_len(__n, _M_get_Tp_allocator()));
 
 _Map_pointer __cur_node;
 __try
@@ -484,6 +484,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
   _M_push_back_aux(const value_type& __t)
 #endif
   {
+   if (size() == max_size())
+ __throw_length_error(
+ __N("cannot create std::deque larger than max_size()"));
+
_M_reserve_map_at_back();
*(this->_M_impl._M_finish._M_node + 1) = this->_M_allocate_node();
__try
@@ -519,6 +523,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
   _M_push_front_aux(const value_type& __t)
 #endif
   {
+   if (size() == max_size())
+ __throw_length_error(
+ __N("cannot create std::deque larger than max_size()"));
+
_M_reserv

Re: [PATCH] treat -Wxxx-larger-than=HWI_MAX special (PR 86631)

[Martin Sebor]

On 08/20/2018 06:14 AM, Richard Biener wrote:

On Thu, Jul 26, 2018 at 10:52 PM Martin Sebor  wrote:


On 07/26/2018 08:58 AM, Martin Sebor wrote:

On 07/26/2018 02:38 AM, Richard Biener wrote:

On Wed, Jul 25, 2018 at 5:54 PM Martin Sebor  wrote:


On 07/25/2018 08:57 AM, Jakub Jelinek wrote:

On Wed, Jul 25, 2018 at 08:54:13AM -0600, Martin Sebor wrote:

I don't mean for the special value to be used except internally
for the defaults.  Otherwise, users wanting to override the default
will choose a value other than it.  I'm happy to document it in
the .opt file for internal users though.

-1 has the documented effect of disabling the warnings altogether
(-1 is SIZE_MAX) so while I agree that -1 looks better it doesn't
work.  (It would need more significant changes.)


The variable is signed, so -1 is not SIZE_MAX.  Even if -1 disables
it, you
could use e.g. -2 or other negative value for the other special case.


The -Wxxx-larger-than=N distinguish three ranges of argument
values (treated as unsigned):

   1.  [0, HOST_WIDE_INT_MAX)
   2.  HOST_WIDE_INT_MAX
   3.  [HOST_WIDE_INT_MAX + 1, Infinity)


But it doesn't make sense for those to be host dependent.


It isn't when the values are handled by each warning.  That's
also the point of this patch: to remove this (unintended)
dependency.


I think numerical user input should be limited to [0, ptrdiff_max]
and cases (1) and (2) should be simply merged, I see no value
in distinguishing them.  -Wxxx-larger-than should be aliased
to [0, ptrdiff_max], case (3) is achieved by -Wno-xxx-larger-than.


To be clear: this is also close to what this patch does.

The only wrinkle is that we don't know the value of PTRDIFF_MAX
either at the time the option initial value is set in the .opt
file or when the option is processed when it's specified either
on the command line or as an alias in the .opt file (as all
-Wno-xxx-larger-than options are).


But then why not make that special value accessible and handle
it as PTRDIFF_MAX when that is available (at users of the params)?

That is,

Index: gcc/calls.c
===
--- gcc/calls.c (revision 262951)
+++ gcc/calls.c (working copy)
@@ -1222,9 +1222,12 @@ alloc_max_size (void)
   if (alloc_object_size_limit)
 return alloc_object_size_limit;

-  alloc_object_size_limit
-= build_int_cst (size_type_node, warn_alloc_size_limit);
+  HOST_WIDE_INT limit = warn_alloc_size_limit;
+  if (limit == HOST_WIDE_INT_MAX)
+limit = tree_to_shwi (TYPE_MAX_VALUE (ptrdiff_type_node));

+  alloc_object_size_limit = build_int_cst (size_type_node, limit);
+
   return alloc_object_size_limit;
 }

use sth like

 if (warn_alloc_size_limit == -1)
   alloc_object_size_limit = fold_convert (size_type_node,
TYPE_MAX_VALUE (ptrdiff_type_node));
 else
   alloc_object_size_limit = size_int (warn_alloc_size_limit);

?  Also removing the need to have > int params values.


Not sure I understand this last part.  Remove the enhancement?
(We do need to handle option arguments in excess of INT_MAX.)



It's that HOST_WIDE_INT_MAX use that is problematic IMHO.  Why not use -1?


-1 is a valid/documented value of the argument of all these
options because it's treated as unsigned HWI:

  Warnings controlled by the option can be disabled either
  by specifying byte-size of ‘SIZE_MAX’

It has an intuitive meaning: warning for any size larger than
the maximum means not warning at all.  Treating -1 as special
instead of HOST_WIDE_INT_MAX would replace that meaning with
"warn on any size in excess of PTRDIFF_MAX."

A reasonable way to disable the warning is like so:

  gcc -Walloc-size-larger-than=$(getconf ULONG_MAX) ...

That would not work anymore.

Treating HOST_WIDE_INT_MAX as PTRDIFF_MAX is the natural choice
on LP64: they have the same value.  It's only less than perfectly
natural in ILP32 and even there it's not a problem in practice
because it's either far out of the range of valid values [0, 4GB]
(i.e., where HWI is a 64-bit long long), or it's also equal to
PTRDIFF_MAX (on hosts with no 64-bit type, if GCC even supports
any).

I'm not trying to be stubborn here but I just don't see why
you think that setting aside HOST_WIDE_INT_MAX is problematic.
Anything else is worse from a user-interface POV.  It makes
little difference inside GCC as long as we want to let users
choose to warn for allocation sizes over some value in
[PTRDIFF_MAX, SIZE_MAX] -- e.g., for malloc() over 3GB but
not for less.  There's also the -Walloca-size-larger-than=
case where PTRDIFF_MAX means only warn for known sizes over
than but not for unknown sizes.

Martin



Richard.


 Case (2) above is only
used by the implementation as a placeholder for PTRDIFF_MAX.
It's not part of the interface -- it's an internal workaround
for lack of a better word.

(There is an additional wrinkle in the -Walloca-larger-than=
has two modes both of which are controlled by a single option:
(a) diagnose only allocations >= PTRDIFF_MAX (defa

Re: [PATCH] Make strlen range computations more conservative

[Jeff Law]

On 08/22/2018 11:22 AM, Bernd Edlinger wrote:
> On 08/22/18 18:05, Martin Sebor wrote:
>> On 08/21/2018 10:05 PM, Bernd Edlinger wrote:
>>> On 08/22/18 00:25, Jeff Law wrote:
 On 08/21/2018 02:43 AM, Richard Biener wrote:
> On Mon, 20 Aug 2018, Bernd Edlinger wrote:
 [ snip. ]

>> Yes, I found some peanuts on my way.
>>
>> For instance this fix for PR middle-end/86121 survives bootstrap on
>> it's own, and fixes one xfail.
>>
>> Is it OK for trunk?
>
> Yes, that's OK for trunk.
 Agreed.  Seems like a nice independent bugfix and I don't think it
 adversely affects anything else under current discussion.  In fact, not
 folding here makes it easier to warn about incorrect code elsewhere.

>
>>> Btw, I don't think we want sth like
>>> flag_assume_zero_terminated_char_arrays or even make it default at
>>> -Ofast.
>>>
>>
>> Yes, I agree.  Is there a consensus about this?
>
> Well, it's my own opinion of course.  Show me a benchmark that
> improves with -fassume-zero-terminated-char-arrays.  Certainly
> for security reasons it sounds a dangerous thing (and the documentation
> needs a more thorough description of what it really means).
 I certainly don't want to see a flag.  We've already got way too many;
 adding another for marginal behavior just seems wrong.

>
>> If yes, I go ahead and remove that option again.
>>
>> BTW: I needed this option in a few test cases, that insist in checking 
>> the
>> optimizer to eliminate stuff, based on the VRP info. (6 +/-1 or so).
>
> Any example?
>
>> But we can as well remove those test cases.
 Bernd, if there are specific tests that you want to see removed, we
 should discuss them.

>>>
>>> The test cases are:
>>> gcc.dg/strlenopt-36.c
>>
>> There are plenty of valid test cases in this test.  For example:
>>
>>    extern char a7[7];
>>    if (strlen (a7) >= 7)   // fold to false
>>      abort ();
>>
>> Even if we wanted to accommodate common definitions the array
>> declarations could be changed to static and the tests would
>> be useful:
>>
>>    static char a7[7];
>>
>> There is no valid program where the if condition could be true.
>>
>>> gcc.dg/strlenopt-40.c
>>
>> There are even more completely uncontroversial test cases here,
>> such as:
>>
>>    if (strlen (i < 0 ? "123" : "4321") > 4)   // fold to false
>>      abort ();
>>
> 
> I see, the trouble is that the test case mixes valid cases with
> cases that depend on type info in GIMPLE.
I believe Martin's point is that there are tests within those files that
are still valid.  We don't want to zap the entire test  unless all the
subtests are invalid.  We need to look at each sub-test and determine if
it's valid or not.

Jeff

Re: [PATCH] Add a character size parameter to c_strlen/get_range_strlen

[Jeff Law]

On 08/22/2018 02:14 AM, Richard Biener wrote:
> On Wed, 22 Aug 2018, Bernd Edlinger wrote:
> 
>> On 08/22/18 09:26, Richard Biener wrote:
>>> On Wed, 22 Aug 2018, Bernd Edlinger wrote:
>>>
 On 08/21/18 10:59, Richard Biener wrote:
> On Tue, 21 Aug 2018, Bernd Edlinger wrote:
>
>> gcc -S -O2 -Wall -Wformat-overflow -ftrack-macro-expansion=0 
>> -fshort-wchar builtin-sprintf-warn-20.c
>> builtin-sprintf-warn-20.c: In function 'test':
>> builtin-sprintf-warn-20.c:19:39: warning: hex escape sequence out of 
>> range
>> 19 | ? (char*)L"\x4142\x4344" : (char*)L"\x41424344\x45464748";
>>   |   ^~~
>>
>> Hmm, this test might create some noise on short-wchar targets.
>>
>> I would prefer a warning here, about the wrong type of the parameter.
>> The buffer overflow is only a secondary thing.
>>
>> For constant objects like those, the GIMPLE type is still guaranteed to 
>> be reliable,
>> right?
>
> TREE_TYPE of tcc_declaration and tcc_constant trees should more-or-less
> (minus qualifications not affecting semantics) be those set by
> frontends.
>

 and in this case:

 const union
 { struct {
   wchar_t x[4];
 };
 struct {
   char z[8];
 };
 } u = {{L"123"}};

 int test()
 {
 return __builtin_strlen(u.z);
 }


 string_constant works out the initializer for u.x
 which has a different type than u.z
>>>
>>> Yes.  That's because it uses ctor-for-folding and friends.  It's
>>> a question of the desired semantics of string_constant whether
>>> it should better return NULL_TREE in this case or whether the
>>> caller has to deal with type mismatches.
>>>
>>
>> Yes, absolutely.
>>
>> c_getstr needs to bail out if the string is not zero-terminated
>> within the limits given by the decl, or the string_cst-type or whatever
>> may help.
>>
>> Furthermore I also consider it possible that the byteoffset
>> is not a multiple of eltsize.  So fail in that case as well.
>>
>>
>> I am currently boot-strapping a patch for this (pr87053):
>> $ cat u.c
>> const union
>> { struct {
>>  char x[4];
>>  char y[4];
>>};
>>struct {
>>  char z[8];
>>};
>> } u = {{"1234", "567"}};
>>
>> int test()
>> {
>>return __builtin_strlen(u.z);
>> }
>>
>> gets folded to 4.
>>
>> ... but unfortunately it will depend on my pr86714 fix which fixes
>> the mem_size parameter returned from string_constant.
>>
>> Frankly, in the moment I feel like I fell in a deep deep hole.   :-O
> 
> /me too with > 100 mails in this and related threads unread ;)
> 
> I thought Jeff applied the mem_size parameter thing but maybe it
> was something else.  *fetches coffee*  Ah, Jeff is still "working"
> on it.
The patch that was applied adds a parameter to c_strlen to indicate how
it should count (ie, bytes, 16bit wchars or 32bit wchars).

There was one hunk that was dependent upon an earlier, more
controversial, patch from Bernd which is still under discussion.  That
hunk was twiddled to preserve the overall goal of Bernd's patch which
added the how to count parameter without being dependent upon the older,
more controversial patch.

It's all confusing and it'd actually help if folks stopped issuing
updated patches which try to handle more cases or which touch on the
areas of code already being looked at.  Otherwise the patches just get
more tangled and we're more likely to end up with developers getting
more entrenched in a particular approach.

Jeff

Re: [PATCH] Check the STRING_CSTs in varasm.c

[Bernd Edlinger]

On 08/22/18 22:54, Martin Sebor wrote:
> On 08/22/2018 08:27 AM, Bernd Edlinger wrote:
>> Hi,
>>
>>
>> this is an updated version of my STRING_CSTs checking in varasm.c
>> patch.
>>
>> It tries to answer the questions that were raised in th GO string literals
>> thread.
>>
>> The answers are:
>> a) strings with TYPE_SIZE_UNIT == NULL do exist, but only for STRING_CSTs
>> in constructors of flexible array struct members.  Not for string literals.
>> b) In all cases where the STRING_CSTs do have a TYPE_SIZE_UNIT, the
>> DECL_SIZE_UNIT has the same value.
>> c) When STRING_CSTs do not have a TYPE_SIZE_UNIT, that is in the constructor
>> of a flexible array member.  In that case the TREE_STRING_LENGTH
>> determines the flexible array size.
>>
>>
>> It changes varasm's get_constant_size to return the TYPE_SIZE_UNIT of
>> a STRING_CST literal as it is, without increasing the storage size
>> to TREE_STRING_LENGTH.  I added an assertion to make sure that
>> all STRING_CSTs have a type size; size == 0 can happen for empty Ada
>> strings.
>>
>> Furthermore it adds code to compare_constant to also compare the
>> STRING_CSTs TYPE_SIZE_UNIT if available.
>>
>> So I want to remove that from get_constant_size in order to not change
>> the memory layout of GO and Ada strings, while still having them look
>> mostly like C string literals.
>>
>> Furthermore I added one more consistency check to check_string_literal,
>> that makes sure that all STRING_CSTs that do have a TYPE_SIZE_UNIT,
>> the size matches the DECL_SIZE_UNIT.
>>
>> Those newly discovered properties of string literals make the code in
>> c_strlen and friends a lot simpler.
>>
>>
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
> 
> 
> @@ -1162,9 +1162,13 @@ These nodes represent string-constants.
> 
>   returns the length of the string, as an @code{int}.  The
> 
>   @code{TREE_STRING_POINTER} is a @code{char*} containing the string
> 
>   itself.  The string may not be @code{NUL}-terminated, and it may contain
> 
> -embedded @code{NUL} characters.  Therefore, the
> 
> -@code{TREE_STRING_LENGTH} includes the trailing @code{NUL} if it is
> 
> -present.
> 
> +embedded @code{NUL} characters.  However, the
> 
> +@code{TREE_STRING_LENGTH} always includes a trailing @code{NUL} that
> 
> +is not part of the language string literal but appended by the front end.
> 
> +If the string shall not be @code{NUL}-terminated the @code{TREE_TYPE}
> 
> 
> If the string is not NUL-terminated (not shall not be -- that
> makes it sound like it must not be nul-terminated).
> 
> +is one character shorter than @code{TREE_STRING_LENGTH}.
> 
> 
> Presumably the phrase "the @code{TREE_TYPE} is shorter" means
> that the type of the string is an array whose domain is
> [0, TREE_STRING_LENGTH - 1], or something like that.  It would
> help to make this clear in a less informal way, especially if
> not all STRING_CST types have a domain (sounds like some don't
> if they have a null TYPE_SIZE_UNIT).
> 

Yes, good point.

> +Excess characters other than one trailing @code{NUL} character are not
> 
> +permitted.
> 
> 
> 
> Does this mean that they can be counted on not to exist
> because the front ends make sure they don't and the middle
> end doesn't create them, or that should not be created but
> that they might still exist?
> 
> You also mentioned a lot of detail in your answers above that
> isn't captured here.  Can that be added?  E.g., the part about
> TYPE_SIZE_UNIT of a STRING_CST being allowed to be null seems
> especially important, as does the bit about flexible array
> members.
>
Yes, initially I just saw the strings which are returned
from string_constant were much longer than the memory they lived in.
Therefore I wrote that patch that shortens the overlength
C strings.  I wanted to keep the property that the bare
string constants are null terminated, and not longer than they
should be, but that the type domain in the array type can
be used to strip off just the wide NUL termination, where the
frontend creates those properly, and the middle-end can
make use of these properties, and varasm.c checks that.

It was Richard's suggestion to add some checking code that the
string constants are actually how we want them to be in varasm.c,
so I did it and discovered a lot of things that are probably
not like we expected them to be.
Some background, and Richards comments are in this thread:
https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01328.html

Richard does not think that it is safe to use the
TYPE_SIZE_UNIT like c_strlen, and other places does, but most
of the time the TYPE_SIZE_UNIT of the STRING_CST is valid.
So I used the assertions in this patch to locate places where
that assumption fails.

This revealed that string literals do always have a type domain,
and it is so far always such that
TYPE_SIZE_UNIT(TREE_TYPE) is something, most of the time = TREE_STRING_LENGTH.

Even if it is smaller than TREE_STRING_LENGTH that does not
matter for varasm, when th

[PATCH] PR libstdc++/87061 remove pmr type aliases for COW strings

[Jonathan Wakely]

The pmr aliases for basic_string and match_results are incompatible with
the gcc4-compatible ABI because the Copy-On-Write basic_string class
doesn't support C++11 allocators.

PR libstdc++/87061
* include/experimental/regex [!_GLIBCXX_USE_CXX11_ABI]
(experimental::pmr::match_results, experimental::pmr::cmatch)
(experimental::pmr::smatch, experimental::pmr::wcmatch)
(experimental::pmr::wsmatch): Do not declare for gcc4-compatible ABI,
because COW strings don't support C++11 allocator model.
* include/experimental/string [!_GLIBCXX_USE_CXX11_ABI]
(experimental::pmr::basic_string, experimental::pmr::string)
(experimental::pmr::u16string, experimental::pmr::u32string)
(experimental::pmr::wstring): Likewise.
* include/std/regex [!_GLIBCXX_USE_CXX11_ABI] (pmr::match_results)
(pmr::cmatch, pmr::smatch, pmr::wcmatch, pmr::wsmatch): Likewise.
* include/std/string [!_GLIBCXX_USE_CXX11_ABI] (pmr::basic_string)
(pmr::string, pmr::u16string, pmr::u32string, pmr::wstring): Likewise.
* testsuite/21_strings/basic_string/types/pmr_typedefs.cc: Require
cxx11-abi.
* testsuite/28_regex/match_results/pmr_typedefs.cc: Likewise.

Tested x86_64-linux, committed to trunk.


commit 134070cf5f9dba6cb655160c551e617d2378cfb4
Author: Jonathan Wakely 
Date:   Wed Aug 22 23:39:31 2018 +0100

PR libstdc++/87061 remove pmr type aliases for COW strings

The pmr aliases for basic_string and match_results are incompatible with
the gcc4-compatible ABI because the Copy-On-Write basic_string class
doesn't support C++11 allocators.

PR libstdc++/87061
* include/experimental/regex [!_GLIBCXX_USE_CXX11_ABI]
(experimental::pmr::match_results, experimental::pmr::cmatch)
(experimental::pmr::smatch, experimental::pmr::wcmatch)
(experimental::pmr::wsmatch): Do not declare for gcc4-compatible 
ABI,
because COW strings don't support C++11 allocator model.
* include/experimental/string [!_GLIBCXX_USE_CXX11_ABI]
(experimental::pmr::basic_string, experimental::pmr::string)
(experimental::pmr::u16string, experimental::pmr::u32string)
(experimental::pmr::wstring): Likewise.
* include/std/regex [!_GLIBCXX_USE_CXX11_ABI] (pmr::match_results)
(pmr::cmatch, pmr::smatch, pmr::wcmatch, pmr::wsmatch): Likewise.
* include/std/string [!_GLIBCXX_USE_CXX11_ABI] (pmr::basic_string)
(pmr::string, pmr::u16string, pmr::u32string, pmr::wstring): 
Likewise.
* testsuite/21_strings/basic_string/types/pmr_typedefs.cc: Require
cxx11-abi.
* testsuite/28_regex/match_results/pmr_typedefs.cc: Likewise.

diff --git a/libstdc++-v3/include/experimental/regex 
b/libstdc++-v3/include/experimental/regex
index eb2af151245..633b396b312 100644
--- a/libstdc++-v3/include/experimental/regex
+++ b/libstdc++-v3/include/experimental/regex
@@ -44,6 +44,7 @@ namespace experimental
 {
 inline namespace fundamentals_v2
 {
+#if _GLIBCXX_USE_CXX11_ABI
 namespace pmr
 {
   template
@@ -57,7 +58,7 @@ namespace pmr
   typedef match_results wsmatch;
 
 } // namespace pmr
-
+#endif
 } // namespace fundamentals_v2
 } // namespace experimental
 _GLIBCXX_END_NAMESPACE_VERSION
diff --git a/libstdc++-v3/include/experimental/string 
b/libstdc++-v3/include/experimental/string
index f101255d641..5a96bf78d73 100644
--- a/libstdc++-v3/include/experimental/string
+++ b/libstdc++-v3/include/experimental/string
@@ -62,6 +62,7 @@ inline namespace fundamentals_v2
   __cont.end());
 }
 
+#if _GLIBCXX_USE_CXX11_ABI
   namespace pmr
   {
 // basic_string using polymorphic allocator in namespace pmr
@@ -77,6 +78,7 @@ inline namespace fundamentals_v2
 typedef basic_string wstring;
 
   } // namespace pmr
+#endif
 } // namespace fundamentals_v2
 } // namespace experimental
 
diff --git a/libstdc++-v3/include/std/regex b/libstdc++-v3/include/std/regex
index f0dfa3f68ab..fd155658353 100644
--- a/libstdc++-v3/include/std/regex
+++ b/libstdc++-v3/include/std/regex
@@ -62,7 +62,7 @@
 #include 
 #include 
 
-#if __cplusplus >= 201703L
+#if __cplusplus >= 201703L && _GLIBCXX_USE_CXX11_ABI
 #include 
 namespace std _GLIBCXX_VISIBILITY(default)
 {
diff --git a/libstdc++-v3/include/std/string b/libstdc++-v3/include/std/string
index d3cc03aa179..dd60df2ba6e 100644
--- a/libstdc++-v3/include/std/string
+++ b/libstdc++-v3/include/std/string
@@ -52,7 +52,7 @@
 #include 
 #include 
 
-#if __cplusplus >= 201703L
+#if __cplusplus >= 201703L && _GLIBCXX_USE_CXX11_ABI
 namespace std _GLIBCXX_VISIBILITY(default)
 {
 _GLIBCXX_BEGIN_NAMESPACE_VERSION
diff --git 
a/libstdc++-v3/testsuite/21_strings/basic_string/types/pmr_typedefs.cc 
b/libstdc++-v3/testsuite/21_strings/basic_string/types/pmr_typedefs.cc
index d20fc42901e..23956673792 100644
--- a/libstdc++-v3/testsu

Re: [PATCH] Make strlen range computations more conservative

[Martin Sebor]

On 08/22/2018 04:34 PM, Jeff Law wrote:

On 08/22/2018 11:22 AM, Bernd Edlinger wrote:

On 08/22/18 18:05, Martin Sebor wrote:

On 08/21/2018 10:05 PM, Bernd Edlinger wrote:

On 08/22/18 00:25, Jeff Law wrote:

On 08/21/2018 02:43 AM, Richard Biener wrote:

On Mon, 20 Aug 2018, Bernd Edlinger wrote:

[ snip. ]


Yes, I found some peanuts on my way.

For instance this fix for PR middle-end/86121 survives bootstrap on
it's own, and fixes one xfail.

Is it OK for trunk?


Yes, that's OK for trunk.

Agreed.  Seems like a nice independent bugfix and I don't think it
adversely affects anything else under current discussion.  In fact, not
folding here makes it easier to warn about incorrect code elsewhere.




Btw, I don't think we want sth like
flag_assume_zero_terminated_char_arrays or even make it default at
-Ofast.



Yes, I agree.  Is there a consensus about this?


Well, it's my own opinion of course.  Show me a benchmark that
improves with -fassume-zero-terminated-char-arrays.  Certainly
for security reasons it sounds a dangerous thing (and the documentation
needs a more thorough description of what it really means).

I certainly don't want to see a flag.  We've already got way too many;
adding another for marginal behavior just seems wrong.




If yes, I go ahead and remove that option again.

BTW: I needed this option in a few test cases, that insist in checking the
optimizer to eliminate stuff, based on the VRP info. (6 +/-1 or so).


Any example?


But we can as well remove those test cases.

Bernd, if there are specific tests that you want to see removed, we
should discuss them.



The test cases are:
gcc.dg/strlenopt-36.c


There are plenty of valid test cases in this test.  For example:

   extern char a7[7];
   if (strlen (a7) >= 7)   // fold to false
 abort ();

Even if we wanted to accommodate common definitions the array
declarations could be changed to static and the tests would
be useful:

   static char a7[7];

There is no valid program where the if condition could be true.


gcc.dg/strlenopt-40.c


There are even more completely uncontroversial test cases here,
such as:

   if (strlen (i < 0 ? "123" : "4321") > 4)   // fold to false
 abort ();



I see, the trouble is that the test case mixes valid cases with
cases that depend on type info in GIMPLE.

I believe Martin's point is that there are tests within those files that
are still valid.  We don't want to zap the entire test  unless all the
subtests are invalid.  We need to look at each sub-test and determine if
it's valid or not.


Right.  And if these changes extend to sprintf as I expect will
be the case there will be many more adjustments to make to those
tests.  Those tests are quite delicate.

As I think Jeff already implied, I would really prefer to tackle
this work myself, both to make sure that it's done without
compromising existing warnings, and that the future enhancements
we have planned in these areas are made possible without too much
churn.

I expect to be able to get to it after I get back from Cauldron.

Martin

Re: [PATCH] Make strlen range computations more conservative

[Bernd Edlinger]

On 08/23/18 00:34, Jeff Law wrote:
> On 08/22/2018 11:22 AM, Bernd Edlinger wrote:
>> On 08/22/18 18:05, Martin Sebor wrote:
>>> On 08/21/2018 10:05 PM, Bernd Edlinger wrote:
 On 08/22/18 00:25, Jeff Law wrote:
> On 08/21/2018 02:43 AM, Richard Biener wrote:
>> On Mon, 20 Aug 2018, Bernd Edlinger wrote:
> [ snip. ]
>
>>> Yes, I found some peanuts on my way.
>>>
>>> For instance this fix for PR middle-end/86121 survives bootstrap on
>>> it's own, and fixes one xfail.
>>>
>>> Is it OK for trunk?
>>
>> Yes, that's OK for trunk.
> Agreed.  Seems like a nice independent bugfix and I don't think it
> adversely affects anything else under current discussion.  In fact, not
> folding here makes it easier to warn about incorrect code elsewhere.
>
>>
 Btw, I don't think we want sth like
 flag_assume_zero_terminated_char_arrays or even make it default at
 -Ofast.

>>>
>>> Yes, I agree.  Is there a consensus about this?
>>
>> Well, it's my own opinion of course.  Show me a benchmark that
>> improves with -fassume-zero-terminated-char-arrays.  Certainly
>> for security reasons it sounds a dangerous thing (and the documentation
>> needs a more thorough description of what it really means).
> I certainly don't want to see a flag.  We've already got way too many;
> adding another for marginal behavior just seems wrong.
>
>>
>>> If yes, I go ahead and remove that option again.
>>>
>>> BTW: I needed this option in a few test cases, that insist in checking 
>>> the
>>> optimizer to eliminate stuff, based on the VRP info. (6 +/-1 or so).
>>
>> Any example?
>>
>>> But we can as well remove those test cases.
> Bernd, if there are specific tests that you want to see removed, we
> should discuss them.
>

 The test cases are:
 gcc.dg/strlenopt-36.c
>>>
>>> There are plenty of valid test cases in this test.  For example:
>>>
>>>     extern char a7[7];
>>>     if (strlen (a7) >= 7)   // fold to false
>>>       abort ();
>>>
>>> Even if we wanted to accommodate common definitions the array
>>> declarations could be changed to static and the tests would
>>> be useful:
>>>
>>>     static char a7[7];
>>>
>>> There is no valid program where the if condition could be true.
>>>
 gcc.dg/strlenopt-40.c
>>>
>>> There are even more completely uncontroversial test cases here,
>>> such as:
>>>
>>>     if (strlen (i < 0 ? "123" : "4321") > 4)   // fold to false
>>>       abort ();
>>>
>>
>> I see, the trouble is that the test case mixes valid cases with
>> cases that depend on type info in GIMPLE.
> I believe Martin's point is that there are tests within those files that
> are still valid.  We don't want to zap the entire test  unless all the
> subtests are invalid.  We need to look at each sub-test and determine if
> it's valid or not.
> 

I can try to just keep test cases like the above, and delete
what doen't work, especially in this test case that seems doable.


Bernd.

Re: [PATCH] Make strlen range computations more conservative

[Bernd Edlinger]

On 08/23/18 00:57, Martin Sebor wrote:
> On 08/22/2018 04:34 PM, Jeff Law wrote:
>> On 08/22/2018 11:22 AM, Bernd Edlinger wrote:
>>> On 08/22/18 18:05, Martin Sebor wrote:
 On 08/21/2018 10:05 PM, Bernd Edlinger wrote:
> On 08/22/18 00:25, Jeff Law wrote:
>> On 08/21/2018 02:43 AM, Richard Biener wrote:
>>> On Mon, 20 Aug 2018, Bernd Edlinger wrote:
>> [ snip. ]
>>
 Yes, I found some peanuts on my way.

 For instance this fix for PR middle-end/86121 survives bootstrap on
 it's own, and fixes one xfail.

 Is it OK for trunk?
>>>
>>> Yes, that's OK for trunk.
>> Agreed.  Seems like a nice independent bugfix and I don't think it
>> adversely affects anything else under current discussion.  In fact, not
>> folding here makes it easier to warn about incorrect code elsewhere.
>>
>>>
> Btw, I don't think we want sth like
> flag_assume_zero_terminated_char_arrays or even make it default at
> -Ofast.
>

 Yes, I agree.  Is there a consensus about this?
>>>
>>> Well, it's my own opinion of course.  Show me a benchmark that
>>> improves with -fassume-zero-terminated-char-arrays.  Certainly
>>> for security reasons it sounds a dangerous thing (and the documentation
>>> needs a more thorough description of what it really means).
>> I certainly don't want to see a flag.  We've already got way too many;
>> adding another for marginal behavior just seems wrong.
>>
>>>
 If yes, I go ahead and remove that option again.

 BTW: I needed this option in a few test cases, that insist in checking 
 the
 optimizer to eliminate stuff, based on the VRP info. (6 +/-1 or so).
>>>
>>> Any example?
>>>
 But we can as well remove those test cases.
>> Bernd, if there are specific tests that you want to see removed, we
>> should discuss them.
>>
>
> The test cases are:
> gcc.dg/strlenopt-36.c

 There are plenty of valid test cases in this test.  For example:

    extern char a7[7];
    if (strlen (a7) >= 7)   // fold to false
  abort ();

 Even if we wanted to accommodate common definitions the array
 declarations could be changed to static and the tests would
 be useful:

    static char a7[7];

 There is no valid program where the if condition could be true.

> gcc.dg/strlenopt-40.c

 There are even more completely uncontroversial test cases here,
 such as:

    if (strlen (i < 0 ? "123" : "4321") > 4)   // fold to false
  abort ();

>>>
>>> I see, the trouble is that the test case mixes valid cases with
>>> cases that depend on type info in GIMPLE.
>> I believe Martin's point is that there are tests within those files that
>> are still valid.  We don't want to zap the entire test  unless all the
>> subtests are invalid.  We need to look at each sub-test and determine if
>> it's valid or not.
> 
> Right.  And if these changes extend to sprintf as I expect will
> be the case there will be many more adjustments to make to those
> tests.  Those tests are quite delicate.
> 

None of these test cases are affected.

> As I think Jeff already implied, I would really prefer to tackle
> this work myself, both to make sure that it's done without
> compromising existing warnings, and that the future enhancements
> we have planned in these areas are made possible without too much
> churn.
> 

Sure, but it is funny, how this patch does not change a single
sprintf warning test case. (a previous version did, but that was
fixed).

> I expect to be able to get to it after I get back from Cauldron.
> 
> Martin

Re: [PATCH] Add a character size parameter to c_strlen/get_range_strlen

[Bernd Edlinger]

On 08/23/18 00:50, Jeff Law wrote:
> On 08/22/2018 02:14 AM, Richard Biener wrote:
>> On Wed, 22 Aug 2018, Bernd Edlinger wrote:
>>
>>> On 08/22/18 09:26, Richard Biener wrote:
 On Wed, 22 Aug 2018, Bernd Edlinger wrote:

> On 08/21/18 10:59, Richard Biener wrote:
>> On Tue, 21 Aug 2018, Bernd Edlinger wrote:
>>
>>> gcc -S -O2 -Wall -Wformat-overflow -ftrack-macro-expansion=0 
>>> -fshort-wchar builtin-sprintf-warn-20.c
>>> builtin-sprintf-warn-20.c: In function 'test':
>>> builtin-sprintf-warn-20.c:19:39: warning: hex escape sequence out of 
>>> range
>>> 19 | ? (char*)L"\x4142\x4344" : (char*)L"\x41424344\x45464748";
>>>|   ^~~
>>>
>>> Hmm, this test might create some noise on short-wchar targets.
>>>
>>> I would prefer a warning here, about the wrong type of the parameter.
>>> The buffer overflow is only a secondary thing.
>>>
>>> For constant objects like those, the GIMPLE type is still guaranteed to 
>>> be reliable,
>>> right?
>>
>> TREE_TYPE of tcc_declaration and tcc_constant trees should more-or-less
>> (minus qualifications not affecting semantics) be those set by
>> frontends.
>>
>
> and in this case:
>
> const union
> { struct {
>wchar_t x[4];
>  };
>  struct {
>char z[8];
>  };
> } u = {{L"123"}};
>
> int test()
> {
>  return __builtin_strlen(u.z);
> }
>
>
> string_constant works out the initializer for u.x
> which has a different type than u.z

 Yes.  That's because it uses ctor-for-folding and friends.  It's
 a question of the desired semantics of string_constant whether
 it should better return NULL_TREE in this case or whether the
 caller has to deal with type mismatches.

>>>
>>> Yes, absolutely.
>>>
>>> c_getstr needs to bail out if the string is not zero-terminated
>>> within the limits given by the decl, or the string_cst-type or whatever
>>> may help.
>>>
>>> Furthermore I also consider it possible that the byteoffset
>>> is not a multiple of eltsize.  So fail in that case as well.
>>>
>>>
>>> I am currently boot-strapping a patch for this (pr87053):
>>> $ cat u.c
>>> const union
>>> { struct {
>>>   char x[4];
>>>   char y[4];
>>> };
>>> struct {
>>>   char z[8];
>>> };
>>> } u = {{"1234", "567"}};
>>>
>>> int test()
>>> {
>>> return __builtin_strlen(u.z);
>>> }
>>>
>>> gets folded to 4.
>>>
>>> ... but unfortunately it will depend on my pr86714 fix which fixes
>>> the mem_size parameter returned from string_constant.
>>>
>>> Frankly, in the moment I feel like I fell in a deep deep hole.   :-O
>>
>> /me too with > 100 mails in this and related threads unread ;)
>>
>> I thought Jeff applied the mem_size parameter thing but maybe it
>> was something else.  *fetches coffee*  Ah, Jeff is still "working"
>> on it.
> The patch that was applied adds a parameter to c_strlen to indicate how
> it should count (ie, bytes, 16bit wchars or 32bit wchars).
> 
> There was one hunk that was dependent upon an earlier, more
> controversial, patch from Bernd which is still under discussion.  That
> hunk was twiddled to preserve the overall goal of Bernd's patch which
> added the how to count parameter without being dependent upon the older,
> more controversial patch.
> 

Which patch do you mean?


> It's all confusing and it'd actually help if folks stopped issuing
> updated patches which try to handle more cases or which touch on the
> areas of code already being looked at.  Otherwise the patches just get
> more tangled and we're more likely to end up with developers getting
> more entrenched in a particular approach.
> 
> Jeff
> 

Re: [PATCH] Add a character size parameter to c_strlen/get_range_strlen

[Jeff Law]

On 08/22/2018 05:22 PM, Bernd Edlinger wrote:
> On 08/23/18 00:50, Jeff Law wrote:
>> On 08/22/2018 02:14 AM, Richard Biener wrote:
>>> On Wed, 22 Aug 2018, Bernd Edlinger wrote:
>>>
 On 08/22/18 09:26, Richard Biener wrote:
> On Wed, 22 Aug 2018, Bernd Edlinger wrote:
>
>> On 08/21/18 10:59, Richard Biener wrote:
>>> On Tue, 21 Aug 2018, Bernd Edlinger wrote:
>>>
 gcc -S -O2 -Wall -Wformat-overflow -ftrack-macro-expansion=0 
 -fshort-wchar builtin-sprintf-warn-20.c
 builtin-sprintf-warn-20.c: In function 'test':
 builtin-sprintf-warn-20.c:19:39: warning: hex escape sequence out of 
 range
 19 | ? (char*)L"\x4142\x4344" : (char*)L"\x41424344\x45464748";
|   ^~~

 Hmm, this test might create some noise on short-wchar targets.

 I would prefer a warning here, about the wrong type of the parameter.
 The buffer overflow is only a secondary thing.

 For constant objects like those, the GIMPLE type is still guaranteed 
 to be reliable,
 right?
>>>
>>> TREE_TYPE of tcc_declaration and tcc_constant trees should more-or-less
>>> (minus qualifications not affecting semantics) be those set by
>>> frontends.
>>>
>>
>> and in this case:
>>
>> const union
>> { struct {
>>wchar_t x[4];
>>  };
>>  struct {
>>char z[8];
>>  };
>> } u = {{L"123"}};
>>
>> int test()
>> {
>>  return __builtin_strlen(u.z);
>> }
>>
>>
>> string_constant works out the initializer for u.x
>> which has a different type than u.z
>
> Yes.  That's because it uses ctor-for-folding and friends.  It's
> a question of the desired semantics of string_constant whether
> it should better return NULL_TREE in this case or whether the
> caller has to deal with type mismatches.
>

 Yes, absolutely.

 c_getstr needs to bail out if the string is not zero-terminated
 within the limits given by the decl, or the string_cst-type or whatever
 may help.

 Furthermore I also consider it possible that the byteoffset
 is not a multiple of eltsize.  So fail in that case as well.


 I am currently boot-strapping a patch for this (pr87053):
 $ cat u.c
 const union
 { struct {
   char x[4];
   char y[4];
 };
 struct {
   char z[8];
 };
 } u = {{"1234", "567"}};

 int test()
 {
 return __builtin_strlen(u.z);
 }

 gets folded to 4.

 ... but unfortunately it will depend on my pr86714 fix which fixes
 the mem_size parameter returned from string_constant.

 Frankly, in the moment I feel like I fell in a deep deep hole.   :-O
>>>
>>> /me too with > 100 mails in this and related threads unread ;)
>>>
>>> I thought Jeff applied the mem_size parameter thing but maybe it
>>> was something else.  *fetches coffee*  Ah, Jeff is still "working"
>>> on it.
>> The patch that was applied adds a parameter to c_strlen to indicate how
>> it should count (ie, bytes, 16bit wchars or 32bit wchars).
>>
>> There was one hunk that was dependent upon an earlier, more
>> controversial, patch from Bernd which is still under discussion.  That
>> hunk was twiddled to preserve the overall goal of Bernd's patch which
>> added the how to count parameter without being dependent upon the older,
>> more controversial patch.
>>
> 
> Which patch do you mean?
This one:

Author: law 
Date:   Thu Aug 16 22:38:04 2018 +

* builtins.c (c_strlen): Add new parameter eltsize.  Use it
for determining how to count the elements.
* builtins.h (c_strlen): Adjust prototype.
* expr.c (string_constant): Add new parameter mem_size.
Set *mem_size appropriately.
* expr.h (string_constant): Adjust protoype.
* gimple-fold.c (get_range_strlen): Add new parameter eltsize.
* gimple-fold.h (get_range_strlen): Adjust prototype.
* gimple-ssa-sprintf.c (get_string_length): Add new
parameter eltsize.
(format_string): Call get_string_length with eltsize.

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@263607
138bc75d-0d04-0410-961f-82ee72b054a4

jeff

Re: [PATCH] Make strlen range computations more conservative

[Jeff Law]

On 08/21/2018 10:15 PM, Bernd Edlinger wrote:
> On 08/22/18 00:43, Jeff Law wrote:
>> [ I'm still digesting, but saw something in this that ought to be broken
>> out... ]
>>
>> On 08/19/2018 09:55 AM, Bernd Edlinger wrote:
>>> diff -Npur gcc/tree-ssa-dse.c gcc/tree-ssa-dse.c
>>> --- gcc/tree-ssa-dse.c  2018-07-18 21:21:34.0 +0200
>>> +++ gcc/tree-ssa-dse.c  2018-08-19 14:29:32.344498771 +0200
>>> @@ -248,6 +248,12 @@ compute_trims (ao_ref *ref, sbitmap live
>>>  residual handling in mem* and str* functions is usually
>>>  reasonably efficient.  */
>>> *trim_tail = last_orig - last_live;
>>> +  /* Don't fold away an out of bounds access, as this defeats proper
>>> +warnings.  */
>>> +  if (*trim_tail
>>> + && compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (ref->base)),
>>> +  last_orig) <= 0)
>>> +   *trim_tail = 0;
>>>   }
>>> else
>>>   *trim_tail = 0;
>> This seems like a good change in and of itself and should be able to go
>> forward without further review work.   Consider this hunk approved,
>> along with any testsuite you have which tickles this code (I didn't
>> immediately see one attached to this patch.  But I could have missed it).
>>
> 
> Sorry, for not being clear on this.
> 
> I needed both hunks "Don't fold away an out of bounds access, as this defeats 
> proper
> warnings" to prevent a regression on gcc.dg/Wstringop-overflow-5.c,
> and surprise surprise, the xfail in gcc.dg/Wstringop-overflow-6.c suddenly 
> popped up.
> 
> So without the unsafe range info, gcc.dg/Wstringop-overflow-5.c needs both 
> hunks
> to not regress, but gcc.dg/Wstringop-overflow-6.c only needs the other one I 
> committed
> yesterday.
> 
> So unfortunately I have no test case except gcc.dg/Wstringop-overflow-5.c for 
> that.
> 
> 
> Still OK?
I almost had a WTF moment mis-parsing Wstringop-overflow-5 thinking it
had no dead stores.  But it's full of dead stores :-)

It sounds like the testsuite will tickle it when the right set of
patches are applied.  THere is no distinct test right now.

I went ahead and bootstrapped/regression tested the DSE change alone and
it doesn't trigger any regressions.  I'm going to install it -- I think
consensus has formed around not folding an out of bounds access so that
we can detect the problem and issue a suitable warning.  One less thing
to keep track of :-)


jeff

[PATCH] Fix tests that fail in C++98 mode

[Jonathan Wakely]

* testsuite/23_containers/deque/capacity/max_size.cc: Fix test for
C++98 mode.
* testsuite/23_containers/deque/modifiers/assign/1.cc: Likewise.
* testsuite/23_containers/list/modifiers/assign/1.cc: Likewise.
* testsuite/23_containers/vector/bool/modifiers/assign/1.cc: Likewise.
* testsuite/23_containers/vector/capacity/max_size.cc: Likewise.
* testsuite/23_containers/vector/modifiers/assign/1.cc: Likewise.

Tested x86_64-linux, committed to trunk.


commit 4eefeb30f431f9c9b138374ce56a6f4a59689164
Author: Jonathan Wakely 
Date:   Thu Aug 23 00:40:58 2018 +0100

Fix tests that fail in C++98 mode

* testsuite/23_containers/deque/capacity/max_size.cc: Fix test for
C++98 mode.
* testsuite/23_containers/deque/modifiers/assign/1.cc: Likewise.
* testsuite/23_containers/list/modifiers/assign/1.cc: Likewise.
* testsuite/23_containers/vector/bool/modifiers/assign/1.cc: 
Likewise.
* testsuite/23_containers/vector/capacity/max_size.cc: Likewise.
* testsuite/23_containers/vector/modifiers/assign/1.cc: Likewise.

diff --git a/libstdc++-v3/testsuite/23_containers/deque/capacity/max_size.cc 
b/libstdc++-v3/testsuite/23_containers/deque/capacity/max_size.cc
index 3dabdd05544..1a38c4ed698 100644
--- a/libstdc++-v3/testsuite/23_containers/deque/capacity/max_size.cc
+++ b/libstdc++-v3/testsuite/23_containers/deque/capacity/max_size.cc
@@ -22,7 +22,7 @@
 #include 
 #include 
 
-using test_type = std::deque;
+typedef std::deque test_type;
 
 typedef test_type::size_type size_type;
 typedef test_type::difference_type difference_type;
diff --git a/libstdc++-v3/testsuite/23_containers/deque/modifiers/assign/1.cc 
b/libstdc++-v3/testsuite/23_containers/deque/modifiers/assign/1.cc
index fbab09b9ba2..a6668f7c199 100644
--- a/libstdc++-v3/testsuite/23_containers/deque/modifiers/assign/1.cc
+++ b/libstdc++-v3/testsuite/23_containers/deque/modifiers/assign/1.cc
@@ -27,7 +27,7 @@ int main()
 {
   std::deque d;
 
-  int array[] { 0, 1, 2 };
+  int array[] = { 0, 1, 2 };
   input_iterator_seq seq(array, array + 3);
 
   d.assign(seq.begin(), seq.end());
diff --git a/libstdc++-v3/testsuite/23_containers/list/modifiers/assign/1.cc 
b/libstdc++-v3/testsuite/23_containers/list/modifiers/assign/1.cc
index c5fde47059a..f4d32883328 100644
--- a/libstdc++-v3/testsuite/23_containers/list/modifiers/assign/1.cc
+++ b/libstdc++-v3/testsuite/23_containers/list/modifiers/assign/1.cc
@@ -27,7 +27,7 @@ int main()
 {
   std::list l;
 
-  int array[] { 0, 1, 2 };
+  int array[] = { 0, 1, 2 };
   input_iterator_seq seq(array, array + 3);
 
   l.assign(seq.begin(), seq.end());
diff --git 
a/libstdc++-v3/testsuite/23_containers/vector/bool/modifiers/assign/1.cc 
b/libstdc++-v3/testsuite/23_containers/vector/bool/modifiers/assign/1.cc
index 833201b39a3..06fb2ab2d03 100644
--- a/libstdc++-v3/testsuite/23_containers/vector/bool/modifiers/assign/1.cc
+++ b/libstdc++-v3/testsuite/23_containers/vector/bool/modifiers/assign/1.cc
@@ -27,7 +27,7 @@ void test01()
 
   std::vector bv;
 
-  bool array[] { false, true, true };
+  bool array[] = { false, true, true };
   input_iterator_seq seq(array, array + 3);
 
   bv.assign(seq.begin(), seq.end());
diff --git a/libstdc++-v3/testsuite/23_containers/vector/capacity/max_size.cc 
b/libstdc++-v3/testsuite/23_containers/vector/capacity/max_size.cc
index 499cd7660f9..34d3c4ab96e 100644
--- a/libstdc++-v3/testsuite/23_containers/vector/capacity/max_size.cc
+++ b/libstdc++-v3/testsuite/23_containers/vector/capacity/max_size.cc
@@ -22,7 +22,7 @@
 #include 
 #include 
 
-using test_type = std::vector;
+typedef std::vector test_type;
 
 typedef test_type::size_type size_type;
 typedef test_type::difference_type difference_type;
diff --git a/libstdc++-v3/testsuite/23_containers/vector/modifiers/assign/1.cc 
b/libstdc++-v3/testsuite/23_containers/vector/modifiers/assign/1.cc
index ca7b125e7ca..001f204c93b 100644
--- a/libstdc++-v3/testsuite/23_containers/vector/modifiers/assign/1.cc
+++ b/libstdc++-v3/testsuite/23_containers/vector/modifiers/assign/1.cc
@@ -27,7 +27,7 @@ void test01()
 
   std::vector v;
 
-  int array[] { 0, 1, 2 };
+  int array[] = { 0, 1, 2 };
   input_iterator_seq seq(array, array + 3);
 
   v.assign(seq.begin(), seq.end());

C++ PATCH for c++/87029, Implement -Wredundant-move

[Marek Polacek]

Now that we have -Wpessimizing-move implemented, it was fairly easy to extend
it to -Wredundant-move, which warns about redundant calls to std::move; it's
supposed to detect cases where the compiler is obliged to treat an object as
if it were an rvalue, so calling std::move is pointless.  Since we implement
Core Issue 1579, it warns in more situations than clang++.  I described this
in more detail in the manual entry.

David, I've also tweaked the warning to use fix-it hints; the point is to turn

  return std::move (x);

into

  return x;

I didn't fool around with looking for the locations of the parens; I've used a
DECL_NAME hack instead.  I've verified it using -fdiagnostics-generate-patch,
which produces e.g.:

@@ -27,7 +27,7 @@
 f ()
 {
   T foo;
-  return std::move (foo);
+  return foo;
 }
 
g++.dg/diagnostic/move1.C tests the fix-it hints for -Wredundant-move and
-Wpessimizing-move.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2018-08-22  Marek Polacek  

PR c++/87029, Implement -Wredundant-move.
* c.opt (Wredundant-move): New option.

* typeck.c (maybe_warn_pessimizing_move): Call convert_from_reference.
Warn about redundant moves.  Use fix-it hints.

* doc/invoke.texi: Document -Wredundant-move.

* g++.dg/cpp0x/Wredundant-move1.C: New test.
* g++.dg/cpp0x/Wredundant-move2.C: New test.
* g++.dg/cpp0x/Wredundant-move3.C: New test.
* g++.dg/cpp0x/Wredundant-move4.C: New test.
* g++.dg/diagnostic/move1.C: New test.

diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index 76840dd77ad..a92be089b40 100644
--- gcc/c-family/c.opt
+++ gcc/c-family/c.opt
@@ -985,6 +985,10 @@ Wredundant-decls
 C ObjC C++ ObjC++ Var(warn_redundant_decls) Warning
 Warn about multiple declarations of the same object.
 
+Wredundant-move
+C++ ObjC++ Var(warn_redundant_move) Warning LangEnabledBy(C++ ObjC++, Wall)
+Warn about redundant calls to std::move.
+
 Wregister
 C++ ObjC++ Var(warn_register) Warning
 Warn about uses of register storage specifier.
diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index 122d9dcd4b3..3f0b263525e 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -9185,7 +9185,7 @@ can_do_nrvo_p (tree retval, tree functype)
 static void
 maybe_warn_pessimizing_move (tree retval, tree functype)
 {
-  if (!warn_pessimizing_move)
+  if (!(warn_pessimizing_move || warn_redundant_move))
 return;
 
   /* C++98 doesn't know move.  */
@@ -9207,14 +9207,31 @@ maybe_warn_pessimizing_move (tree retval, tree functype)
  STRIP_NOPS (arg);
  if (TREE_CODE (arg) == ADDR_EXPR)
arg = TREE_OPERAND (arg, 0);
+ arg = convert_from_reference (arg);
  /* Warn if we could do copy elision were it not for the move.  */
  if (can_do_nrvo_p (arg, functype))
{
  auto_diagnostic_group d;
- if (warning_at (location_of (retval), OPT_Wpessimizing_move,
- "moving a local object in a return statement "
- "prevents copy elision"))
-   inform (location_of (retval), "remove % call");
+ gcc_rich_location richloc (location_of (retval));
+ tree name = DECL_NAME (arg);
+ richloc.add_fixit_replace (IDENTIFIER_POINTER (name));
+ warning_at (&richloc, OPT_Wpessimizing_move,
+ "moving a local object in a return statement "
+ "prevents copy elision");
+   }
+ /* Warn if the move is redundant.  It is redundant when we would
+do maybe-rvalue overload resolution even without std::move.  */
+ else if (((VAR_P (arg) && !DECL_HAS_VALUE_EXPR_P (arg))
+   || TREE_CODE (arg) == PARM_DECL)
+  && DECL_CONTEXT (arg) == current_function_decl
+  && !TREE_STATIC (arg))
+   {
+ auto_diagnostic_group d;
+ gcc_rich_location richloc (location_of (retval));
+ tree name = DECL_NAME (arg);
+ richloc.add_fixit_replace (IDENTIFIER_POINTER (name));
+ warning_at (&richloc, OPT_Wredundant_move,
+ "redundant move in return statement");
}
}
 }
diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
index e4148297a87..341499f49b7 100644
--- gcc/doc/invoke.texi
+++ gcc/doc/invoke.texi
@@ -231,7 +231,7 @@ in the following sections.
 -Wdelete-non-virtual-dtor -Wdeprecated-copy  -Wliteral-suffix @gol
 -Wmultiple-inheritance @gol
 -Wnamespaces  -Wnarrowing @gol
--Wpessimizing-move @gol
+-Wpessimizing-move  -Wredundant-move @gol
 -Wnoexcept  -Wnoexcept-type  -Wclass-memaccess @gol
 -Wnon-virtual-dtor  -Wreorder  -Wregister @gol
 -Weffc++  -Wstrict-null-sentinel  -Wtemplates @gol
@@ -3158,6 +3158,49 @@ But in this example, the @code{std::move} call prevents 
copy elision.
 
 This warning is enabled by @option{-Wall}.
 
+@item -Wno-redundant-move @r{(C++ and Objective-C++

Re: [PATCH] Signed zero for {max,min}val intrinsics

[Jerry DeLisle]

On 08/22/2018 01:06 PM, Janne Blomqvist wrote:

The Fortran standard specifies (e.g. F2018 7.4.3.2) that intrinsic
procedures shall treat positive and negative real zero as equivalent,
unless it is explicitly specified otherwise.  For {max,min}val there
is no such explicit mention.  Thus, remove code to handle signed
zeros.

Regtested on x86_64-pc-linux-gnu, Ok for trunk?


Looks good, OK.

Thanks,

Jerry

---snip---

Re: [PATCH v3 01/10] Initial TI PRU GCC port

[Dimitar Dimitrov]

On Monday, 8/20/2018 11:12:23 EEST Jeff Law wrote:
> On 08/15/2018 10:49 PM, Dimitar Dimitrov wrote:
> > ChangeLog:
> > 
> > 2018-07-27  Dimitar Dimitrov  
> > 
> > * configure: Regenerate.
> > * configure.ac: Add PRU target.
> > 
> > gcc/ChangeLog:
> > 
> > 2018-07-27  Dimitar Dimitrov  
> > 
> > * common/config/pru/pru-common.c: New file.
> > * config.gcc: Add PRU target.
> > * config/pru/alu-zext.md: New file.
> > * config/pru/constraints.md: New file.
> > * config/pru/predicates.md: New file.
> > * config/pru/pru-opts.h: New file.
> > * config/pru/pru-passes.c: New file.
> > * config/pru/pru-pragma.c: New file.
> > * config/pru/pru-protos.h: New file.
> > * config/pru/pru.c: New file.
> > * config/pru/pru.h: New file.
> > * config/pru/pru.md: New file.
> > * config/pru/pru.opt: New file.
> > * config/pru/t-pru: New file.
> > * doc/extend.texi: Document PRU pragmas.
> > * doc/invoke.texi: Document PRU-specific options.
> > * doc/md.texi: Document PRU asm constraints.
> > 
> > Signed-off-by: Dimitar Dimitrov 
> > 
> > diff --git a/gcc/config/pru/alu-zext.md b/gcc/config/pru/alu-zext.md
> > new file mode 100644
> > index 000..2112b08d3f4
> > --- /dev/null
> > +++ b/gcc/config/pru/alu-zext.md
> 
> [ ... ]
> I wonder if the zero-extensions in alu-zext.md are really needed.  Can
> we define WORD_REGISTER_OPERATIONS for PRU?  Or do you really want to
> expose sub-word operations?  I simply don't know enough about the
> hardware to provide guidance here.  But it might allow for some
> simplification of the port.  You wouldn't want to define sub-word
> patterns such as addqi/addhi anymore either.
The QI/HI ALU patterns are needed for efficient code generation and ABI 
compliance.

In the PRU GCC port, UNITS_PER_WORD=1. Hence addqi is a full word size 
pattern, and WORD_REGISTER_OPERATIONS should not matter.

Note that in PRU architecture, ALU operands can be either 8, 16 or 32 bits. 
All inputs are zero-extended to 32 bits, and outputs are truncated as needed. 
Here is a relevant discussion: http://gcc.gnu.org/ml/gcc/2017-01/msg00217.html

For example, the following C snippet:
  uint16_t test(uint8_t a, uint16_t b)
  {
return a + b;
  }


Produces the following RTL and efficient assembler output:

  (insn 13 8 14 2 (set (reg/i:HI 56 r14.b0)
  (plus:HI (zero_extend:HI (reg:QI 56 r14.b0 [ a ]))
 (reg:HI 57 r14.b1 [ b ]))) "test.c":407 76 )


  test:
# Note: Function arguments are passed in sub-hw-registers
add r14.w0, r14.b0, r14.w1
ret

> 
> > diff --git a/gcc/config/pru/pru.c b/gcc/config/pru/pru.c
> > new file mode 100644
> > index 000..de72d22278e
> > --- /dev/null
> > +++ b/gcc/config/pru/pru.c
> > @@ -0,0 +1,3001 @@
> > 
> > +/* Emit efficient RTL equivalent of ADD3 with the given const_int for
> > +   frame-related registers.
> > + op0 - Destination register.
> > + op1 - First addendum operand (a register).
> > + addendum - Second addendum operand (a constant).
> > + kind- Note kind.  REG_NOTE_MAX if no note must be added.
> > + reg_note_rtx - Reg note RTX.  NULL if it should be computed
> > automatically. + */
> > +static rtx
> > +pru_add3_frame_adjust (rtx op0, rtx op1, int addendum,
> > +  const enum reg_note kind, rtx reg_note_rtx)
> > +{
> > +  rtx insn;
> > +
> > +  rtx op0_adjust = gen_rtx_SET (op0, plus_constant (Pmode, op1,
> > addendum)); +
> > +  if (UBYTE_INT (addendum) || UBYTE_INT (-addendum))
> > +insn = emit_insn (op0_adjust);
> > +  else
> > +{
> > +  /* Help the compiler to cope with an arbitrary integer constant.
> > +Reload has finished so we can't expect the compiler to
> > +auto-allocate a temporary register.  But we know that call-saved
> > +registers are not live yet, so we utilize them.  */
> 
> Note I think this can run afoul of IPA-RA.  THough it looks like you're
> exposing this in RTL, so you're probably safe.
Grepping for PROLOGUE_TEMP_REG, it appears that MIPS and Risc-V use the same 
technique to get a temporary register for prologue/epilougue.

> 
> > +/* Emit function epilogue.  */
> > +void
> > +pru_expand_epilogue (bool sibcall_p)
> > +{
> > +  rtx cfa_adj;
> > +  int total_frame_size;
> > +  int sp_adjust, save_offset;
> > +  int regno_start;
> > +
> > +  if (!sibcall_p && pru_can_use_return_insn ())
> > +{
> > +  emit_jump_insn (gen_return ());
> > +  return;
> > +}
> > +
> > +  emit_insn (gen_blockage ());
> > +
> > +  total_frame_size = pru_compute_frame_layout ();
> > +  if (frame_pointer_needed)
> > +{
> > +  /* Recover the stack pointer.  */
> > +  cfa_adj = plus_constant (Pmode, stack_pointer_rtx,
> > +  (total_frame_size
> > +   - cfun->machine->save_regs_offset));
> > +  pru_add3_frame_adjust (stack_pointer_rtx,
> > +hard_frame_pointer_rtx,
> > + 

Re: [PATCH] Add a character size parameter to c_strlen/get_range_strlen

[Bernd Edlinger]

On 08/23/18 01:36, Jeff Law wrote:
> On 08/22/2018 05:22 PM, Bernd Edlinger wrote:
>> On 08/23/18 00:50, Jeff Law wrote:
>>> On 08/22/2018 02:14 AM, Richard Biener wrote:
 On Wed, 22 Aug 2018, Bernd Edlinger wrote:

> On 08/22/18 09:26, Richard Biener wrote:
>> On Wed, 22 Aug 2018, Bernd Edlinger wrote:
>>
>>> On 08/21/18 10:59, Richard Biener wrote:
 On Tue, 21 Aug 2018, Bernd Edlinger wrote:

> gcc -S -O2 -Wall -Wformat-overflow -ftrack-macro-expansion=0 
> -fshort-wchar builtin-sprintf-warn-20.c
> builtin-sprintf-warn-20.c: In function 'test':
> builtin-sprintf-warn-20.c:19:39: warning: hex escape sequence out of 
> range
> 19 | ? (char*)L"\x4142\x4344" : (char*)L"\x41424344\x45464748";
> |   
> ^~~
>
> Hmm, this test might create some noise on short-wchar targets.
>
> I would prefer a warning here, about the wrong type of the parameter.
> The buffer overflow is only a secondary thing.
>
> For constant objects like those, the GIMPLE type is still guaranteed 
> to be reliable,
> right?

 TREE_TYPE of tcc_declaration and tcc_constant trees should more-or-less
 (minus qualifications not affecting semantics) be those set by
 frontends.

>>>
>>> and in this case:
>>>
>>> const union
>>> { struct {
>>> wchar_t x[4];
>>>   };
>>>   struct {
>>> char z[8];
>>>   };
>>> } u = {{L"123"}};
>>>
>>> int test()
>>> {
>>>   return __builtin_strlen(u.z);
>>> }
>>>
>>>
>>> string_constant works out the initializer for u.x
>>> which has a different type than u.z
>>
>> Yes.  That's because it uses ctor-for-folding and friends.  It's
>> a question of the desired semantics of string_constant whether
>> it should better return NULL_TREE in this case or whether the
>> caller has to deal with type mismatches.
>>
>
> Yes, absolutely.
>
> c_getstr needs to bail out if the string is not zero-terminated
> within the limits given by the decl, or the string_cst-type or whatever
> may help.
>
> Furthermore I also consider it possible that the byteoffset
> is not a multiple of eltsize.  So fail in that case as well.
>
>
> I am currently boot-strapping a patch for this (pr87053):
> $ cat u.c
> const union
> { struct {
>char x[4];
>char y[4];
>  };
>  struct {
>char z[8];
>  };
> } u = {{"1234", "567"}};
>
> int test()
> {
>  return __builtin_strlen(u.z);
> }
>
> gets folded to 4.
>
> ... but unfortunately it will depend on my pr86714 fix which fixes
> the mem_size parameter returned from string_constant.
>
> Frankly, in the moment I feel like I fell in a deep deep hole.   :-O

 /me too with > 100 mails in this and related threads unread ;)

 I thought Jeff applied the mem_size parameter thing but maybe it
 was something else.  *fetches coffee*  Ah, Jeff is still "working"
 on it.
>>> The patch that was applied adds a parameter to c_strlen to indicate how
>>> it should count (ie, bytes, 16bit wchars or 32bit wchars).
>>>
>>> There was one hunk that was dependent upon an earlier, more
>>> controversial, patch from Bernd which is still under discussion.  That
>>> hunk was twiddled to preserve the overall goal of Bernd's patch which
>>> added the how to count parameter without being dependent upon the older,
>>> more controversial patch.
>>>
>>
>> Which patch do you mean?
> This one:
> 
> Author: law 
> Date:   Thu Aug 16 22:38:04 2018 +
> 
>  * builtins.c (c_strlen): Add new parameter eltsize.  Use it
>  for determining how to count the elements.
>  * builtins.h (c_strlen): Adjust prototype.
>  * expr.c (string_constant): Add new parameter mem_size.
>  Set *mem_size appropriately.
>  * expr.h (string_constant): Adjust protoype.
>  * gimple-fold.c (get_range_strlen): Add new parameter eltsize.
>  * gimple-fold.h (get_range_strlen): Adjust prototype.
>  * gimple-ssa-sprintf.c (get_string_length): Add new
> parameter eltsize.
>  (format_string): Call get_string_length with eltsize.
> 
>  git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@263607
> 138bc75d-0d04-0410-961f-82ee72b054a4
> 

Yes, and which one was the earlier, more controversial patch from me?


Thanks
Bernd.