Re: [PATCH] Added information about inline assembler in stack calculations (.su files)

[Niklas DAHLQUIST]

On 12/1/18 1:15 AM, Jeff Law wrote:
> On 11/26/18 7:02 AM, Torbjorn SVENSSON wrote:
>> Hi,
>>
>> Attached is a small patch that, in case of inline assembler code,
>> indicates that the function stack usage is uncertain due to inline
>> assembler.
>>
>> The test suite are using "nop" as an assembler instruction on all
>> targets, is this acceptable or is there a better way to test this?
>>
>> Patch has been tested on gcc-arm-none-eabi-7-2018-q2-update for both
>> arm-none-eabi and x86_64-linux-gnu and SVN head (r266454) for
>> x86_64-linux-gnu.
> One could argue that allocating stack space inside an ASM is a really
> bad idea.  Consider things like dwarf debugging and unwind tables.  If
> you're allocating stack inside an ASM that stuff is going to be totally
> wrong.
>
> So I think my question before moving forward with something like this is
> whether or not it makes sense at all to bother dumping data for a
> scenario that we'd probably suggest developers avoid to begin with.
>
> jeff
Hi,

The purpose of the patch is to notify when the reported stack usage might be
incorrect. Even if it's bad practice to alter stack in asm, there are 
use cases
in the embedded world that makes sense. A notable common use case is 
FreeRTOS
task switch using ARM "naked" attribute and inline asm, which reports "0 
static",
which gives a faulty stack usage. We have considered the other option to
report a warning for these cases, but that alternative hasn't appealed 
to us.

Niklas

Re: [PATCH, Fortran] pad char to int conversions with spaces instead of zeros (legacy)

[Jerry DeLisle]

On 12/6/18 2:33 AM, Jakub Jelinek wrote:

On Wed, Dec 05, 2018 at 06:27:00PM -0800, Jerry DeLisle wrote:

I disagree completely. I assume the idea of -fdec-pad-with-spaces is to
accomodate some old dec fortran code. The only reason to use some other
character is if someone is writing new dec fortran code, which is implying
encouraging people to be writing non standard conforming code.

Even if it is conforming in the sense that it is processor dependent you are
encouraging people to create new non portable code across compilers. Please
just stay consistent with Intel.


So do you prefer to always use ' ' instead of '\0', or decide based on -fdec
without a separate option controlling that?



Keep current default of '\0' and use ' ' when -fdec given.

Regards,

Jerry

Re: Fortran patches

[Steve Kargl]

On Thu, Dec 06, 2018 at 05:21:32PM -0800, Steve Kargl wrote:
> 
> Here's an alternative patch that would reject a subroutine
> with an alternate return dummy argument with the bind(c)
> attributes.  I'm still trying to determine if the code 
> should be legal.  The c.l.f thread I started isn't helping :(

I think I have found the restriction.  In F2018,

C1554  If proc-language-binding-spec is specified for a procedure, each
   of its dummy arguments shall be an interoperable procedure (18.3.7)
   or a variable that is interoperable (18.3.5, 18.3.6), assumed-shape,
   assumed-rank, assumed-type, of type CHARACTER with assumed length,
   or that has the ALLOCATABLE or POINTER attribute.


> 
> Index: decl.c
> ===
> --- decl.c(revision 266766)
> +++ decl.c(working copy)
> @@ -7467,6 +7467,7 @@ gfc_match_subroutine (void)
>match is_bind_c;
>char peek_char;
>bool allow_binding_name;
> +  locus loc;
>  
>if (gfc_current_state () != COMP_NONE
>&& gfc_current_state () != COMP_INTERFACE
> @@ -7532,6 +7533,8 @@ gfc_match_subroutine (void)
>/* Here, we are just checking if it has the bind(c) attribute, and if
>   so, then we need to make sure it's all correct.  If it doesn't,
>   we still need to continue matching the rest of the subroutine line.  */
> +  gfc_gobble_whitespace ();
> +  loc = gfc_current_locus;
>is_bind_c = gfc_match_bind_c (sym, allow_binding_name);
>if (is_bind_c == MATCH_ERROR)
>  {
> @@ -7543,6 +7546,8 @@ gfc_match_subroutine (void)
>  
>if (is_bind_c == MATCH_YES)
>  {
> +  gfc_formal_arglist *arg;
> +
>/* The following is allowed in the Fortran 2008 draft.  */
>if (gfc_current_state () == COMP_CONTAINS
> && sym->ns->proc_name->attr.flavor != FL_MODULE
> @@ -7556,8 +7561,17 @@ gfc_match_subroutine (void)
>gfc_error ("Missing required parentheses before BIND(C) at %C");
>return MATCH_ERROR;
>  }
> -  if (!gfc_add_is_bind_c (&(sym->attr), sym->name,
> -   &(sym->declared_at), 1))
> +
> +  /* Scan the dummy arguments for an alternate return.  */
> +  for (arg = sym->formal; arg; arg = arg->next)
> + if (!arg->sym)
> +   {
> + gfc_error ("Alternate return dummy argument cannot appear in a "
> +"SUBROUTINE with the BIND(C) attribute at %L", );
> + return MATCH_ERROR;
> +   }
> +
> +  if (!gfc_add_is_bind_c (&(sym->attr), sym->name, &(sym->declared_at), 
> 1))
>  return MATCH_ERROR;
>  }
> 
> -- 
> steve

-- 
Steve
20170425 https://www.youtube.com/watch?v=VWUpyCsUKR4
20161221 https://www.youtube.com/watch?v=IbCHE-hONow

Re: [PATCH] PR86957

[Indu Bhagat]

On 12/05/2018 03:33 AM, Thomas Schwinge wrote:

Hi!

Sorry for my late follow-up; had a lot of catch up to do back then.

On Thu, 27 Sep 2018 11:47:31 +0200, Richard Biener  
wrote:

On Mon, Sep 24, 2018 at 9:14 PM Indu Bhagat  wrote:

Done. Attached is updated patch.

Patch is tested on x86_64

You obviously did_not_  properly test the patch since it causes a
bunch of new testsuite
failures: [...]

and more.  Please get up to speed with GCC development and testing requirements!

Also, with this commit, several test cases regressed from PASS to
UNSUPPORTED because of "{ dg-require-effective-target freorder }" running
into:

 fprofile_use_freorder16732.c: In function 'void foo()':
 fprofile_use_freorder16732.c:2:20: warning: 
'[...]/gcc/testsuite/g++/fprofile_use_freorder16732.gcda' profile count data 
file not found [-Wmissing-profile]

Iain had just mentioned that in, and fixed
in r266785,.
(Back then, I had produced the same fix, but not yet posted.)  :-|

With "{ dg-require-effective-target freorder }" thusly restored, I then
also saw two other regressions/FAILs, back then:

 [-UNSUPPORTED: g++.dg/tree-prof/pr63581.C-]
 UNSUPPORTED: g++.dg/tree-prof/pr63581.C -fauto-profile
 {+PASS: g++.dg/tree-prof/pr63581.C compilation,  -fprofile-generate 
-D_PROFILE_GENERATE+}
 {+FAIL: g++.dg/tree-prof/pr63581.C compilation,  -fprofile-use 
-D_PROFILE_USE+}
 {+PASS: g++.dg/tree-prof/pr63581.C execution,-fprofile-generate 
-D_PROFILE_GENERATE+}
 {+UNRESOLVED: g++.dg/tree-prof/pr63581.C execution,-fprofile-use 
-D_PROFILE_USE+}
 
 [...]/gcc/testsuite/g++.dg/tree-prof/pr63581.C: In function 'int uptodate(page*)':

 [...]/gcc/testsuite/g++.dg/tree-prof/pr63581.C:28:19: warning: profile for 
function 'int uptodate(page*)' not found in profile data [-Wmissing-profile]

..., and:

 [-UNSUPPORTED: gcc.dg/tree-prof/20041218-1.c-]
 UNSUPPORTED: gcc.dg/tree-prof/20041218-1.c -fauto-profile
 {+PASS: gcc.dg/tree-prof/20041218-1.c compilation,  -fprofile-generate 
-D_PROFILE_GENERATE+}
 {+FAIL: gcc.dg/tree-prof/20041218-1.c compilation,  -fprofile-use 
-D_PROFILE_USE+}
 {+PASS: gcc.dg/tree-prof/20041218-1.c execution,-fprofile-generate 
-D_PROFILE_GENERATE+}
 {+UNRESOLVED: gcc.dg/tree-prof/20041218-1.c execution,-fprofile-use 
-D_PROFILE_USE+}
 
 [...]/gcc/testsuite/gcc.dg/tree-prof/20041218-1.c: In function 'bar':

 [...]/gcc/testsuite/gcc.dg/tree-prof/20041218-1.c:58:1: warning: profile 
for function 'bar' not found in profile data [-Wmissing-profile]

..., for which I came up with the following patch.

But, this doesn't now seem to be necessary anymore, or am I confused?
Maybe this got fixed differently -- or is anybody still seeing these
FAILs?  (If not, then I'm not proposing to commit this, of course.)


I tested the trunk as of commit 85df98d7fbb6ef5c5b6e97190fff4028f4b70747
(Dec 5 PR c/87028) with and without the -Wmissing-profile on by default
in the compiler. I.e., with the following change :

Common Var(warn_missing_profile) Init(1) Warning
+Common Var(warn_missing_profile) Init(0) Warning

Two observations :
1. The two issues (tree-prof/pr63581.C and tree-prof/20041218-1.c) that you run
   into are not repeatable on my end. I am not sure why you run into them.

2. I do however see other tests (a total of 23) which are have regressed from
   PASS --> UNRESOLVED. A diff is attached.

   Each one of them is due to "Error/Warning threshold exceeded:  1 0 (max. 1 
3)"

E.g.,
ERROR: gcc.dg/tree-prof/20050826-2.c: error executing dg-final-use: couldn't open 
"20050826-2.c.116t.dom2 20050826-2.c.115t.dom2": no such file or directory
  Error/Warning threshold exceeded:  1 0 (max. 1 3)

(I have not been able to see the -Wmissing-profile warning explicitly in my logs
 yet. I was thinking a RUNTESTFLAGS="-v -v -v" should have helped, but it 
didnt.)

Any suggestions to resolve these tests are appreciated. I am not sure yet of the
most maintainable way to "allow the warning in all tests that it appears". I
need to look around a bit (Is Thomas' patch doing just what needs to be done 
here ?).

Thanks
Indu

UNRESOLVED: c-c++-common/unroll-1.c  -Wc++-compat : error executing dg-final: 
couldn't open "unroll-1.c.250r.loop2_unroll unroll-1.c.251r.loop2_unroll": no 
such file or directory
UNRESOLVED: gcc.dg/ipa/ipa-icf-38.c: error executing dg-final: couldn't open 
"ipa-icf-38.exe.wpa.069i.icf ipa-icf-38.exe.wpa.070i.icf": no such file or 
directory
UNRESOLVED: gcc.dg/tree-prof/20050826-2.c: Error executing dg-final-use: 
couldn't open "20050826-2.c.116t.dom2 20050826-2.c.115t.dom2": no such file or 
directory
UNRESOLVED: gcc.dg/tree-prof/cmpsf-1.c: Error executing dg-final-use: couldn't 
open "cmpsf-1.c.115t.dom2 cmpsf-1.c.116t.dom2": no such file or directory
UNRESOLVED: gcc.dg/tree-prof/inliner-1.c: Error executing dg-final-use: 
couldn't open 

Re: Fortran patches

[Steve Kargl]

On Thu, Dec 06, 2018 at 02:08:54PM -0500, Fritz Reese wrote:
> On Wed, Dec 5, 2018 at 7:03 PM Steve Kargl
> >
> > > RE:
> > > >PR fortran/88139
> > > >* dump-parse-tree.c (write_proc): Alternate return.
> > > I dissent with this patch. The introduced error is meaningless and, as
> > > mentioned by comment #3 in the PR, avoiding the ICE in dump-parse-tree
> > > is not directly the issue. The code should be rejected in parsing. In
> > > gcc-8.1 the invalid code is accepted (without an ICE) even without the
> > > -fc-prototypes flag: I haven't finished building the compiler with
> > > your changes yet to see whether that is still true afterwards, but at
> > > least the test case doesn't try this, so I strongly suspect the patch
> > > is incomplete to fix the PR.
> >
> > Comment #3 does not contain a patch to fix the problem elsewhere.
> >
> > In F2003, 15.2.6 "Interoperability of procedures and procedure interfaces",
> > I cannot find a prohibition on an alternate return in a subroutine
> > interface with BIND(C).
> >
> > I'm disinclined to let a patch fester in bugzilla to only attain
> > the same fate as my patch to PR68544.
> 
> According to F2008 §15.3.7.2(5):
> 
> > any dummy argument without the VALUE attribute [...] is interoperable with 
> > an entity of the
> > referenced type (ISO/IEC 9899:1999, 6.2.5, 7.17, and 7.18.1) of the formal 
> > parameter
> 
> Regardless of whether or not we accept alternate returns in BIND(C)
> procedures, the compiler must be at least consistent: if we accept
> them (which gfortran currently does), then we should be able to dump
> the C prototype (with -fc-prototypes), providing a formal parameter
> interoperable with the type of the alternate return dummy argument; if
> we reject them, then we should issue the error in parsing (before
> handling by -fc-prototypes). In either case, the error message should
> not be obscure or meaningless. Even so, the patch here is inconsistent
> since we accept the code, but issue an error when attempting to dump
> the C prototype.

Here's an alternative patch that would reject a subroutine
with an alternate return dummy argument with the bind(c)
attributes.  I'm still trying to determine if the code 
should be legal.  The c.l.f thread I started isn't helping :(

Index: decl.c
===
--- decl.c  (revision 266766)
+++ decl.c  (working copy)
@@ -7467,6 +7467,7 @@ gfc_match_subroutine (void)
   match is_bind_c;
   char peek_char;
   bool allow_binding_name;
+  locus loc;
 
   if (gfc_current_state () != COMP_NONE
   && gfc_current_state () != COMP_INTERFACE
@@ -7532,6 +7533,8 @@ gfc_match_subroutine (void)
   /* Here, we are just checking if it has the bind(c) attribute, and if
  so, then we need to make sure it's all correct.  If it doesn't,
  we still need to continue matching the rest of the subroutine line.  */
+  gfc_gobble_whitespace ();
+  loc = gfc_current_locus;
   is_bind_c = gfc_match_bind_c (sym, allow_binding_name);
   if (is_bind_c == MATCH_ERROR)
 {
@@ -7543,6 +7546,8 @@ gfc_match_subroutine (void)
 
   if (is_bind_c == MATCH_YES)
 {
+  gfc_formal_arglist *arg;
+
   /* The following is allowed in the Fortran 2008 draft.  */
   if (gfc_current_state () == COMP_CONTAINS
  && sym->ns->proc_name->attr.flavor != FL_MODULE
@@ -7556,8 +7561,17 @@ gfc_match_subroutine (void)
   gfc_error ("Missing required parentheses before BIND(C) at %C");
   return MATCH_ERROR;
 }
-  if (!gfc_add_is_bind_c (&(sym->attr), sym->name,
- &(sym->declared_at), 1))
+
+  /* Scan the dummy arguments for an alternate return.  */
+  for (arg = sym->formal; arg; arg = arg->next)
+   if (!arg->sym)
+ {
+   gfc_error ("Alternate return dummy argument cannot appear in a "
+  "SUBROUTINE with the BIND(C) attribute at %L", );
+   return MATCH_ERROR;
+ }
+
+  if (!gfc_add_is_bind_c (&(sym->attr), sym->name, &(sym->declared_at), 1))
 return MATCH_ERROR;
 }

-- 
steve

Re: [doc,committed] clarify docs for function attribute "const"

[Martin Sebor]

On 12/3/18 11:04 PM, Sandra Loosemore wrote:

On 12/3/18 2:47 PM, Martin Sebor wrote:

[snip]

Attached is my proposed update.  The user's email suggested going
into a lot of detail that I'm not sure would be helpful.  I think
it's safer to keep it simple than to try to carefully outline tricky
conditions under which some const or pure functions might get away
with modifying program state.  At the same time, I tried not to
outright prohibit it so I phrased the restrictions in terms of
observable program state (as opposed to reading or writing
"global" variables and such).

I also made a few other changes, like move the example of
the square function from pure to const (since the function is
much more likely const), and made the const restrictions stand
on their own, rather than depending on pure.



Index: /ssd/src/gcc/svn/gcc/doc/extend.texi
===
--- /ssd/src/gcc/svn/gcc/doc/extend.texi    (revision 266760)
+++ /ssd/src/gcc/svn/gcc/doc/extend.texi    (working copy)
@@ -2518,25 +2518,45 @@ are automatically detected and this attribute 
is i

 @item const
 @cindex @code{const} function attribute
 @cindex functions that have no side effects
-Many functions do not examine any values except their arguments, and
-have no effects except to return a value.  Calls to such functions lend
-themselves to optimization such as common subexpression elimination.
-The presence of the @code{const} attribute on a function declaration 
-allows GCC to emit more efficient code for some calls to the 
function. +Calls to functions whose return value is not affected by 
changes to

+the observable state of the program and that have no observable effects
+on such state other than to return a value may lend themselves to
+optimizations such as common subexpression elimination.  Declaring such
+functions with the @code{const} attribute allows GCC to emit more 
efficient

+code for consecutive calls to the function.


I don't think the calls have to be consecutive for GCC to emit more 
efficient code.  I'd kill that last bit.


I added it because I want to make it clear that the attribute
shouldn't be expected to improve the efficiency of any single
call.  It can only improve it for the second (and subsequent)
call with the same argument values as the first.  I've tweaked
the sentence a bit.




+The @code{const} attribute prohibits a function from reading objects
+that affect its return value between successive invocations.  However,
+functions declared with the attribute can safely read objects that do
+not affect their return value, such as non-volatile constants.
 The @code{const} attribute imposes greater restrictions on a function's
-definition than the similar @code{pure} attribute below because it
-additionally prohibits the function from reading memory except for
-constant global variables.  Decorating the same function with
-both the @code{const} and the @code{pure} attribute is diagnosed.
+definition than the similar @code{pure} attribute.  Declaring the same
+function with both the @code{const} and the @code{pure} attribute is
+diagnosed.  Because a const function cannot have any observable side


@code{const}


I actually meant to bring this up separately.

I don't think @code would be appropriate in this context.  It's
not a use of the attribute name but rather a description of
the function.  Analogous to "a const pointer" or "a volatile
access" vs "a @code{const}-qualified pointer or
"the @code{volatile} qualifier."  I think the manual already
uses this style, albeit not completely consistently.  The C
and C++ standards aren't 100% consistent either, but I believe
the editors follow this principle when they think of it.  Unless
you really do think GCC should go the other way around I would
like to add this to the GCC style guidelines and fix
the inconsistencies.

I thought I was consistent in this in my own changes but I see
I was not.  I've changed the @code{const} to just const according
to the above.


+effects it does not make sense for it to return @code{void}.  Declaring
+such a function is diagnosed.

+For example,
+
+@smallexample
+int square (int) __attribute__ ((const));
+@end smallexample
+
+@noindent
+tells GCC that subsequent calls to function @code{square} with the same
+argument value can be replaced by the result of the first call 
regardless

+of the statements in between.
+


I think it would be better to move this example immediately after the 
first paragraph, before you digress about the diagnostics and such like.


Done.




 @cindex pointer arguments
 Note that a function that has pointer arguments and examines the data
-pointed to must @emph{not} be declared @code{const}.  Likewise, a
-function that calls a non-@code{const} function usually must not be
-@code{const}.  Because a @code{const} function cannot have any side
-effects it does not make sense for such a function to return 
@code{void}.

-Declaring such a function is 

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

[Iain Sandoe]

Hi

This got stuck in my stack of patches for LTO debug support, and I forgot to 
ping it…

> On 22 Aug 2018, at 14:20, Richard Biener  wrote:
> 
> 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.

>> 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_.

So I just look for the LTO_SECTION_NAME_PREFIX

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

In this version, I look for the OFFLOAD_SECTION_NAME_PREFIX as well,
although AFAICS collect2 itself has no specific handling of offload, the 
detection
is done in lto-wrapper.

OK for trunk now?

thanks
Iain


From: Iain Sandoe 
Date: Tue, 21 Aug 2018 12:55:02 +0100
Subject: [PATCH] [PATCH, collect2] Use simple-object to find out if objects
 contain LTO.

This replaces the use of nm to search for the LTO common symbol marker
and uses simple object to see if there's a section starting with
".gnu.lto_." or .gnu.offload_lto_
---
 gcc/collect2.c | 120 +++--
 1 file changed, 57 insertions(+), 63 deletions(-)

diff --git a/gcc/collect2.c b/gcc/collect2.c
index 60269682ec..dcbd3e18a6 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.  */
@@ -1710,7 +1715,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);
 
return 0;
   }
@@ -1780,7 

[C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)

[Alexandre Oliva]

This patch started out from the testcase in PR88146, that attempted to
synthesize an inherited ctor without any args before a varargs
ellipsis and crashed while at that, because of the unguarded
dereferencing of the parm type list, that usually contains a
terminator.  The terminator is not there for varargs functions,
however, and without any other args, we ended up dereferencing a NULL
pointer.  Oops.

Guarding the accesses there was easy, but I missed the sorry message
we got in other testcases that passed arguments through the ellipsis
in inherited ctors.  I put a check in, and noticed the inherited ctors
were synthesized with the location assigned to the class name,
although they were initially assigned the location of the using
declaration.  I decided the latter was better, and arranged for the
better location to be retained.

Further investigation revealed the lack of a sorry message had to do
with the call being in a non-evaluated context, in this case, a
noexcept expression.  The sorry would be correctly reported in other
contexts, so I rolled back the check I'd added, but retained the
source location improvement.

I was still concerned about issuing sorry messages while instantiating
template ctors even in non-evaluated contexts, e.g., if a template
ctor had a base initializer that used an inherited ctor with enough
arguments that they'd go through an ellipsis.  I wanted to defer the
instantiation of such template ctors, but that would have been wrong
for constexpr template ctors, and already done for non-constexpr ones.
So, I just consolidated multiple test variants into a single testcase
that explores and explains various of the possibilities I thought of.

Regstrapped on x86_64- and i686-linux-gnu, mistakenly along with a patch
with a known regression, and got only that known regression.  Retesting
without it.  Ok to install?


for  gcc/cp/ChangeLog

PR c++/88146
* method.c (do_build_copy_constructor): Do not crash with
ellipsis-only parm list.
(synthesize_method): Retain location of inherited ctor.

for  gcc/testsuite/ChangeLog

PR c++/88146
* g++.dg/cpp0x/inh-ctor32.C: New.
---
 gcc/cp/method.c |9 +
 gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C |  229 +++
 2 files changed, 234 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C

diff --git a/gcc/cp/method.c b/gcc/cp/method.c
index fd023e200538..41d609fb1de6 100644
--- a/gcc/cp/method.c
+++ b/gcc/cp/method.c
@@ -643,7 +643,7 @@ do_build_copy_constructor (tree fndecl)
   bool trivial = trivial_fn_p (fndecl);
   tree inh = DECL_INHERITED_CTOR (fndecl);
 
-  if (!inh)
+  if (parm && !inh)
 parm = convert_from_reference (parm);
 
   if (trivial)
@@ -677,7 +677,7 @@ do_build_copy_constructor (tree fndecl)
 {
   tree fields = TYPE_FIELDS (current_class_type);
   tree member_init_list = NULL_TREE;
-  int cvquals = cp_type_quals (TREE_TYPE (parm));
+  int cvquals = parm ? cp_type_quals (TREE_TYPE (parm)) : 0;
   int i;
   tree binfo, base_binfo;
   tree init;
@@ -891,8 +891,9 @@ synthesize_method (tree fndecl)
 
   /* Reset the source location, we might have been previously
  deferred, and thus have saved where we were first needed.  */
-  DECL_SOURCE_LOCATION (fndecl)
-= DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (fndecl)));
+  if (!DECL_INHERITED_CTOR (fndecl))
+DECL_SOURCE_LOCATION (fndecl)
+  = DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (fndecl)));
 
   /* If we've been asked to synthesize a clone, just synthesize the
  cloned function instead.  Doing so will automatically fill in the
diff --git a/gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C 
b/gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C
new file mode 100644
index ..c40412fc5346
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C
@@ -0,0 +1,229 @@
+// { dg-do compile { target c++11 } }
+// Minimized from the testcase for PR c++/88146,
+// then turned into multiple variants. 
+
+// We issue an error when calling an inherited ctor with at least one
+// argument passed through a varargs ellipsis, if the call is in an
+// evaluated context.  Even in nonevaluated contexts, we will
+// instantiate constexpr templates (unlike non-constexpr templates),
+// which might then issue errors that in nonevlauated contexts
+// wouldn't be issued.
+
+// In these variants, the inherited ctor is constexpr, but it's only
+// called in unevaluated contexts, so no error is issued.  The
+// templateness of the ctor doesn't matter, because the only call that
+// passes args through the ellipsis is in a noexcept expr, that is not
+// evaluated.  The ctors in derived classes are created and
+// instantiated, discarding arguments passed through the ellipsis when
+// calling base ctors, but that's not reported: we only report a
+// problem when *calling* ctors that behave this way.
+namespace unevaled_call {
+  

[C++ PATCH] [PR c++/87814] undefer deferred noexcept on tsubst if request

[Alexandre Oliva]

tsubst_expr and tsubst_copy_and_build are not expected to handle
DEFERRED_NOEXCEPT exprs, but if tsubst_exception_specification takes a
DEFERRED_NOEXCEPT expr with !defer_ok, it just passes the expr on for
tsubst_copy_and_build to barf.

This patch arranges for tsubst_exception_specification to combine the
incoming args with those already stored in a DEFERRED_NOEXCEPT, and
then substitute them into the pattern, when retaining a deferred
noexcept is unacceptable.

Regstrapped on x86_64- and i686-linux-gnu, mistakenly along with a patch
with a known regression, and got only that known regression.  Retesting
without it.  Ok to install?


for  gcc/cp/ChangeLog

PR c++/87814
* pt.c (tsubst_exception_specification): Handle
DEFERRED_NOEXCEPT with !defer_ok.

for  gcc/testsuite/ChangeLog

PR c++/87814
* g++.dg/cpp1z/pr87814.C: New.
---
 gcc/cp/pt.c  |   14 +++---
 gcc/testsuite/g++.dg/cpp1z/pr87814.C |   26 ++
 2 files changed, 37 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1z/pr87814.C

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 8560e5885933..72ae7173d92c 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -14150,9 +14150,17 @@ tsubst_exception_specification (tree fntype,
}
}
   else
-   new_specs = tsubst_copy_and_build
- (expr, args, complain, in_decl, /*function_p=*/false,
-  /*integral_constant_expression_p=*/true);
+   {
+ if (DEFERRED_NOEXCEPT_SPEC_P (specs))
+   {
+ args = add_to_template_args (DEFERRED_NOEXCEPT_ARGS (expr),
+  args);
+ expr = DEFERRED_NOEXCEPT_PATTERN (expr);
+   }
+ new_specs = tsubst_copy_and_build
+   (expr, args, complain, in_decl, /*function_p=*/false,
+/*integral_constant_expression_p=*/true);
+   }
   new_specs = build_noexcept_spec (new_specs, complain);
 }
   else if (specs)
diff --git a/gcc/testsuite/g++.dg/cpp1z/pr87814.C 
b/gcc/testsuite/g++.dg/cpp1z/pr87814.C
new file mode 100644
index ..37034bb58cd6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/pr87814.C
@@ -0,0 +1,26 @@
+// { dg-do compile { target c++17 } }
+
+template
+struct box {
+template
+constexpr box(E && e)
+noexcept(noexcept(Element(e)))
+{}
+};
+
+template
+struct compressed_tuple_ : box ... {
+template
+constexpr compressed_tuple_(Args &&... args)
+noexcept((noexcept(box(args)) && ...))
+  : box(args)...
+{}
+};
+
+struct adaptor_cursor : compressed_tuple_ {
+using compressed_tuple_::compressed_tuple_;
+};
+
+int main() {
+(void)noexcept(adaptor_cursor{(int*)0});
+}

-- 
Alexandre Oliva, freedom fighter   https://FSFLA.org/blogs/lxo
Be the change, be Free! FSF Latin America board member
GNU Toolchain EngineerFree Software Evangelist
Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe

Re: [committed] Add test for PR85770.

[Jakub Jelinek]

On Wed, Dec 05, 2018 at 05:41:49PM -0700, Jeff Law wrote:
> 
> PR85770 is fixed by Segher's combiner patch to avoid combining hard
> regs.  Presumably it helps because it gives the allocators more freedom.
> 
> I'm adding the testcase from the PR to the regression suite.
> 
> Jeff

> commit 40fc691eac0ea9414f7908826c91afc70ff78617
> Author: law 
> Date:   Thu Dec 6 00:40:08 2018 +
> 
> PR rtl-optimization/85770
> * gcc.target/i386/pr85770.c: New test.

The test FAILs on i686-linux, because __int128 is not supported on 32-bit
targets.

Fixed thusly, tested on x86_64-linux and i686-linux, committed as obvious to
trunk.

2018-12-07  Jakub Jelinek  

PR rtl-optimization/85770
* gcc.target/i386/pr85770.c: Require int128 effective target.

--- gcc/testsuite/gcc.target/i386/pr85770.c (revision 266876)
+++ gcc/testsuite/gcc.target/i386/pr85770.c (working copy)
@@ -1,4 +1,5 @@
-/* { dg-do compile } */
+/* PR rtl-optimization/85770 */
+/* { dg-do compile { target int128 } } */
 /* { dg-options "-O2 -march=nano-1000 -fnon-call-exceptions 
-fno-tree-coalesce-vars" } */
 
 unsigned a, b, c, d, e, f, g, h, i;
@@ -14,4 +15,3 @@ __int128 foo(char k, unsigned short l, u
   return k + l + m + n + o + a + b + c + d + j + l + e + f + q + 4294967295 +
  p + g + h + i;
 }
-


Jakub

Re: [PATCH][RFC] Poison bitmap_head->obstack

[Martin Sebor]

On 12/5/18 7:58 AM, Richard Biener wrote:

On Wed, 5 Dec 2018, Jeff Law wrote:


On 12/4/18 6:16 AM, Richard Biener wrote:


This tries to make bugs like that in PR88317 harder to create by
introducing a bitmap_release function that can be used as
pendant to bitmap_initialize for non-allocated bitmap heads.
The function makes sure to poison the bitmaps obstack member
so the obstack the bitmap was initialized with can be safely
released.

The patch also adds a default constructor to bitmap_head
doing the same, but for C++ reason initializes to a
all-zero bitmap_obstack rather than 0xdeadbeef because
the latter isn't possible in constexpr context (it is
by using unions but then things start to look even more ugly).

The stage1 compiler might end up with a few extra runtime
initializers but constexpr makes sure they'll vanish for
later stages.

I had to paper over that you-may-not-use-memset-to-zero classes
with non-trivial constructors warning in two places and I
had to teach gengtype about CONSTEXPR (probably did so in
an awkward way - suggestions and pointers into gengtype
appreciated).

Bootstrapped (with host GCC 4.8) on x86_64-unknown-linux-gnu,
testing in progress.

The LRA issue seems to be rare enough (on x86_64...) that
I didn't trip over it sofar.

Comments?  Do we want this?  Not sure how we can easily
discover all bitmap_clear () users that should really
use bitmap_release (suggestion for a better name appreciated
as well - I thought about bitmap_uninitialize)

Richard.

2018-12-04  Richard Biener  

* bitmap.c (bitmap_head::crashme): Define.
* bitmap.h (bitmap_head): Add constexpr default constructor
poisoning the obstack member.
(bitmap_head::crashme): Declare.
(bitmap_release): New function clearing a bitmap and poisoning
the obstack member.
* gengtype.c (main): Make it recognize CONSTEXPR.

* lra-constraints.c (lra_inheritance): Use bitmap_release
instead of bitmap_clear.

* ira.c (ira): Work around warning.
* regrename.c (create_new_chain): Likewise.

I don't see enough complexity in here to be concerning -- so if it makes
it harder to make mistakes, then I'm for it.


Any comment about the -Wclass-memaccess workaround sprinkling around two
(void *) conversions?  I didn't dig deep enough to look for a more
appropriate solution, also because there were some issues with older
host compilers and workarounds we installed elsewhere...


Using '*head = du_head ();' is the solution I like to encourage
to zero out typed objects.  When the memory isn't initialized
and the type has a user-defined ctor, bypassing it is undefined
even if the ctor is a no-op (the ctor starts the lifetime of
the object).  In that case, placement new is the appropriate
way to bring the object to life and value-initialize it:

  new (head) du_head ();

If that's not good enough or portable enough to ancient compilers
then I would suggest adding a comment to explain the intent of
the cast.

Martin



Otherwise yes, it makes it harder to do mistakes.  I'll probably
use bitmap_head::crashme instead of 0xdeadbeef in bitmap_release.
And of course we'd need to hunt down users of bitmap_clear that
should be bitmap_release instead...

Thanks,
Richard.

Re: [PATCH] [PR86823] retain deferred access checks from outside firewall

[Alexandre Oliva]

On Dec  5, 2018, Jason Merrill  wrote:

> Hmm, I'm uncomfortable with how this depends on the specific
> implementation of tentative_firewall.

*nod*

> What do you think of this alternate approach (untested other than with
> the testcase)?

If that won't drop any other deferred access checks that we might want
to keep when leaving the firewall by other paths, it looks much nicer
indeed!  I had been careful to only move access checks when I was
certain about the replacement, but even there I was concerned about
carrying stuff along with the preparsed token that didn't belong.

I'm giving your proposed patch a full round of testing along with other
patches.

-- 
Alexandre Oliva, freedom fighter   https://FSFLA.org/blogs/lxo
Be the change, be Free! FSF Latin America board member
GNU Toolchain EngineerFree Software Evangelist
Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe

Re: [PATCH] Disable -fipa-ra for naked functions (PR target/85593)

[Jeff Law]

On 12/6/18 4:34 PM, Jakub Jelinek wrote:
> Hi!
> 
> The only documented supported content of naked functions is basic asm
> statement(s).  Those don't have clobbers though, so we should ignore
> naked functions for IPA-RA; if they are written the only supported way,
> they will appear not to clobber any registers at all and IPA-RA will then
> assume they don't clobber any registers.
> While naked is a target attribute supported only on a subset of targets,
> the generic code already handles it in multiple spots, so I think we can add
> another spot rather than introducing a target hook for it.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2018-12-07  Jakub Jelinek  
> 
>   PR target/85593
>   * final.c (rest_of_handle_final): Don't call collect_fn_hard_reg_usage
>   for functions with naked attribute.
> 
>   * gcc.target/i386/pr85593.c: New test.
OK
jeff

[PATCH] Disable -fipa-ra for naked functions (PR target/85593)

[Jakub Jelinek]

Hi!

The only documented supported content of naked functions is basic asm
statement(s).  Those don't have clobbers though, so we should ignore
naked functions for IPA-RA; if they are written the only supported way,
they will appear not to clobber any registers at all and IPA-RA will then
assume they don't clobber any registers.
While naked is a target attribute supported only on a subset of targets,
the generic code already handles it in multiple spots, so I think we can add
another spot rather than introducing a target hook for it.

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

2018-12-07  Jakub Jelinek  

PR target/85593
* final.c (rest_of_handle_final): Don't call collect_fn_hard_reg_usage
for functions with naked attribute.

* gcc.target/i386/pr85593.c: New test.

--- gcc/final.c.jj  2018-11-21 19:38:50.264064648 +0100
+++ gcc/final.c 2018-12-06 19:01:39.052525201 +0100
@@ -4659,7 +4659,11 @@ rest_of_handle_final (void)
   final_start_function_1 (, asm_out_file, , optimize);
   final_1 (first, asm_out_file, seen, optimize);
   if (flag_ipa_ra
-  && !lookup_attribute ("noipa", DECL_ATTRIBUTES (current_function_decl)))
+  && !lookup_attribute ("noipa", DECL_ATTRIBUTES (current_function_decl))
+  /* Functions with naked attributes are supported only with basic asm
+statements in the body, thus for supported use cases the information
+on clobbered registers is not available.  */
+  && !lookup_attribute ("naked", DECL_ATTRIBUTES (current_function_decl)))
 collect_fn_hard_reg_usage ();
   final_end_function ();
 
--- gcc/testsuite/gcc.target/i386/pr85593.c.jj  2018-12-06 19:19:17.286362641 
+0100
+++ gcc/testsuite/gcc.target/i386/pr85593.c 2018-12-06 19:18:47.188850564 
+0100
@@ -0,0 +1,30 @@
+/* PR target/85593 */
+/* { dg-do run { target { { i?86-*-linux* x86_64-*-linux* } && lp64 } } } */
+/* { dg-options "-O2" } */
+
+__attribute__((naked)) void
+bar (void)
+{
+  asm ("xorl %eax, %eax\n\t"
+   "xorl %edx, %edx\n\t"
+   "xorl %ecx, %ecx\n\t"
+   "xorl %esi, %esi\n\t"
+   "xorl %edi, %edi\n\t"
+   "xorl %r8d, %r8d\n\t"
+   "xorl %r9d, %r9d\n\t"
+   "xorl %r10d, %r10d\n\t"
+   "xorl %r11d, %r11d\n\t"
+   "ret");
+}
+
+int
+main ()
+{
+  int a = 42;
+  asm ("" : "+r" (a));
+  bar ();
+  asm ("" : "+r" (a));
+  if (a != 42)
+__builtin_abort ();
+  return 0;
+}

Jakub

[committed] Fix OpenMP handling of character allocatable scalars (PR fortran/88377)

[Jakub Jelinek]

Hi!

Apparently the length decls corresponding to allocatable scalars with
character type are also GFC_DECL_GET_SCALAR_ALLOCATABLE; the OpenMP
clause handling code was relying on those to have POINTER_TYPEs, but
the lengths are integrals and should be handled normally.

Bootstrapped/regtested on x86_64-linux and i686-linux, committed to trunk.

2018-12-07  Jakub Jelinek  

PR fortran/88377
* trans-openmp.c (gfc_omp_clause_default_ctor,
gfc_omp_clause_copy_ctor, gfc_omp_clause_assign_op,
gfc_omp_clause_linear_ctor, gfc_omp_clause_dtor): Only consider
GFC_DECL_GET_SCALAR_ALLOCATABLE vars as scalar allocatables if they
have pointer type.

* gfortran.dg/gomp/pr88377.f90: New test.

--- gcc/fortran/trans-openmp.c.jj   2018-11-08 18:07:56.253072145 +0100
+++ gcc/fortran/trans-openmp.c  2018-12-06 15:58:54.647301230 +0100
@@ -460,7 +460,8 @@ gfc_omp_clause_default_ctor (tree clause
 
   if ((! GFC_DESCRIPTOR_TYPE_P (type)
|| GFC_TYPE_ARRAY_AKIND (type) != GFC_ARRAY_ALLOCATABLE)
-  && !GFC_DECL_GET_SCALAR_ALLOCATABLE (OMP_CLAUSE_DECL (clause)))
+  && (!GFC_DECL_GET_SCALAR_ALLOCATABLE (OMP_CLAUSE_DECL (clause))
+ || !POINTER_TYPE_P (type)))
 {
   if (gfc_has_alloc_comps (type, OMP_CLAUSE_DECL (clause)))
{
@@ -567,7 +568,8 @@ gfc_omp_clause_copy_ctor (tree clause, t
 
   if ((! GFC_DESCRIPTOR_TYPE_P (type)
|| GFC_TYPE_ARRAY_AKIND (type) != GFC_ARRAY_ALLOCATABLE)
-  && !GFC_DECL_GET_SCALAR_ALLOCATABLE (OMP_CLAUSE_DECL (clause)))
+  && (!GFC_DECL_GET_SCALAR_ALLOCATABLE (OMP_CLAUSE_DECL (clause))
+ || !POINTER_TYPE_P (type)))
 {
   if (gfc_has_alloc_comps (type, OMP_CLAUSE_DECL (clause)))
{
@@ -667,7 +669,8 @@ gfc_omp_clause_assign_op (tree clause, t
 
   if ((! GFC_DESCRIPTOR_TYPE_P (type)
|| GFC_TYPE_ARRAY_AKIND (type) != GFC_ARRAY_ALLOCATABLE)
-  && !GFC_DECL_GET_SCALAR_ALLOCATABLE (OMP_CLAUSE_DECL (clause)))
+  && (!GFC_DECL_GET_SCALAR_ALLOCATABLE (OMP_CLAUSE_DECL (clause))
+ || !POINTER_TYPE_P (type)))
 {
   if (gfc_has_alloc_comps (type, OMP_CLAUSE_DECL (clause)))
{
@@ -905,7 +908,8 @@ gfc_omp_clause_linear_ctor (tree clause,
 
   if ((! GFC_DESCRIPTOR_TYPE_P (type)
|| GFC_TYPE_ARRAY_AKIND (type) != GFC_ARRAY_ALLOCATABLE)
-  && !GFC_DECL_GET_SCALAR_ALLOCATABLE (OMP_CLAUSE_DECL (clause)))
+  && (!GFC_DECL_GET_SCALAR_ALLOCATABLE (OMP_CLAUSE_DECL (clause))
+ || !POINTER_TYPE_P (type)))
 {
   gcc_assert (TREE_CODE (type) == ARRAY_TYPE);
   if (!TYPE_DOMAIN (type)
@@ -989,7 +993,8 @@ gfc_omp_clause_dtor (tree clause, tree d
 
   if ((! GFC_DESCRIPTOR_TYPE_P (type)
|| GFC_TYPE_ARRAY_AKIND (type) != GFC_ARRAY_ALLOCATABLE)
-  && !GFC_DECL_GET_SCALAR_ALLOCATABLE (OMP_CLAUSE_DECL (clause)))
+  && (!GFC_DECL_GET_SCALAR_ALLOCATABLE (OMP_CLAUSE_DECL (clause))
+ || !POINTER_TYPE_P (type)))
 {
   if (gfc_has_alloc_comps (type, OMP_CLAUSE_DECL (clause)))
return gfc_walk_alloc_comps (decl, NULL_TREE,
--- gcc/testsuite/gfortran.dg/gomp/pr88377.f90.jj   2018-12-06 
16:00:28.779775783 +0100
+++ gcc/testsuite/gfortran.dg/gomp/pr88377.f90  2018-12-06 16:00:02.714198182 
+0100
@@ -0,0 +1,15 @@
+! PR fortran/88377
+! { dg-do compile }
+
+program pr88377
+  call s(3)
+contains
+  subroutine s(n)
+integer :: n
+character(n), allocatable :: x
+x = 'abc'
+!$omp task
+print *, x, (x == 'abc')
+!$omp end task
+  end
+end

Jakub

[C++ PATCH] Fix make_temporary_var_for_ref_to_temp (PR c++/86669)

[Jakub Jelinek]

On Wed, Dec 05, 2018 at 09:50:56PM +0100, Jakub Jelinek wrote:
> On Wed, Dec 05, 2018 at 03:49:26PM -0500, Jason Merrill wrote:
> > On 11/28/18 3:42 AM, Jakub Jelinek wrote:
> > > Whenever we need to clone a cdtor (either because the target doesn't 
> > > support
> > > aliases the way we need, e.g. initlist105.C testcase on darwin, where it 
> > > has
> > > been originally reported, or when it has virtual bases, like the made up
> > > initlist106.C on x86_64-linux), we rely on DECL_INITIAL of the local
> > > variables being unshared, because the tree unsharing gimplify.c performs
> > > doesn't unshare DECL_INITIAL.  clone_body has some code to recurse on the
> > > DECL_INITIAL, but it handles just decls FOR_EACH_LOCAL_DECL walks.  I
> > > believe it is generally ok that not all temporaries are covered in local
> > > decls, the gimplifier should take care of those fine if we don't need
> > > debug info for them.
> > 
> > I think any temporaries that have DECL_INITIAL should be pushed so that they
> > end up in local_decls.  set_up_extended_ref_temp already adds a DECL_EXPR
> > for it, I guess we need a pushdecl as well.  Though the comment for
> > get_temp_regvar suggests that this is problematic somehow.
> 
> Ok, will play with it tomorrow.

The following fixes the testcase too and passed bootstrap/regtest on
x86_64-linux and i686-linux, ok for trunk?

2018-12-07  Jakub Jelinek  

PR c++/86669
* call.c (make_temporary_var_for_ref_to_temp): Call pushdecl even for
automatic vars.

* g++.dg/cpp0x/initlist105.C: New test.
* g++.dg/cpp0x/initlist106.C: New test.
* g++.dg/other/pr86669.C: New test.

--- gcc/cp/call.c.jj2018-11-29 23:11:38.386646583 +0100
+++ gcc/cp/call.c   2018-12-06 13:50:26.766178596 +0100
@@ -11130,14 +11130,12 @@ make_temporary_var_for_ref_to_temp (tree
   tree name = mangle_ref_init_variable (decl);
   DECL_NAME (var) = name;
   SET_DECL_ASSEMBLER_NAME (var, name);
-
-  var = pushdecl (var);
 }
   else
 /* Create a new cleanup level if necessary.  */
 maybe_push_cleanup_level (type);
 
-  return var;
+  return pushdecl (var);
 }
 
 /* EXPR is the initializer for a variable DECL of reference or
--- gcc/testsuite/g++.dg/cpp0x/initlist105.C.jj 2018-12-06 13:31:35.993609689 
+0100
+++ gcc/testsuite/g++.dg/cpp0x/initlist105.C2018-12-06 13:31:35.993609689 
+0100
@@ -0,0 +1,28 @@
+// PR c++/86669
+// { dg-do run { target c++11 } }
+
+#include 
+
+struct S { S (); };
+struct T : public S {};
+int cnt;
+void foo (int) { cnt++; }
+
+S::S ()
+{
+  int e = 1, f = 2, g = 3, h = 4;
+
+  for (auto k : { e, f, g, h })
+foo (k);
+}
+
+int
+main ()
+{
+  S s;
+  if (cnt != 4)
+__builtin_abort ();
+  T t;
+  if (cnt != 8)
+__builtin_abort ();
+}
--- gcc/testsuite/g++.dg/cpp0x/initlist106.C.jj 2018-12-06 13:31:35.993609689 
+0100
+++ gcc/testsuite/g++.dg/cpp0x/initlist106.C2018-12-06 13:31:35.993609689 
+0100
@@ -0,0 +1,29 @@
+// PR c++/86669
+// { dg-do run { target c++11 } }
+
+#include 
+
+struct A { };
+struct S : virtual public A { S (); };
+struct T : public S, virtual public A {};
+int cnt;
+void foo (int) { cnt++; }
+
+S::S ()
+{
+  int e = 1, f = 2, g = 3, h = 4;
+
+  for (auto k : { e, f, g, h })
+foo (k);
+}
+
+int
+main ()
+{
+  S s;
+  if (cnt != 4)
+__builtin_abort ();
+  T t;
+  if (cnt != 8)
+__builtin_abort ();
+}
--- gcc/testsuite/g++.dg/other/pr86669.C.jj 2018-12-06 13:31:35.993609689 
+0100
+++ gcc/testsuite/g++.dg/other/pr86669.C2018-12-06 13:31:35.993609689 
+0100
@@ -0,0 +1,10 @@
+// PR c++/86669
+// { dg-do compile }
+
+struct S { S (); };
+struct T : public S {};
+
+S::S ()
+{
+  int *p = { (int *)  };
+}


Jakub

Re: [PATCH] handle function pointers in __builtin_object_size (PR 88372)

[Martin Sebor]

On 12/6/18 2:26 PM, Jakub Jelinek wrote:

On Thu, Dec 06, 2018 at 01:21:58PM -0700, Martin Sebor wrote:

Bug 88372 - alloc_size attribute is ignored on function pointers
points out that even though the alloc_size attribute is accepted
on function pointers it doesn't have any effect on Object Size
Checking.  The reporter, who is implementing the feature in Clang,
wants to know if by exposing it under the same name they won't be
causing incompatibilities with GCC.

I don't think it's intentional that GCC doesn't take advantage of
the attribute for Object Size Checking, and certainly not to detect
the same kinds of issues as with other allocation functions (such
as excessive or negative size arguments).  Rather, it's almost
certainly an oversight since GCC does make use of function pointer
attributes in other contexts (e.g., attributes alloc_align and
noreturn).

As an oversight, I think it's fair to consider it a bug rather
than a request for an enhancement.  Since not handling
the attribute in Object Size Checking has adverse security
implications, I also think this bug should be addressed in GCC
9.  With that, I submit the attached patch to resolve both
aspects of the problem.


This is because alloc_object_size has been written before we had attributes
like alloc_size.  The only thing I'm unsure about is whether we should
prefer gimple_call_fntype or TREE_TYPE (gimple_call_fndecl ()) if it is a
direct call or if we should try to look for alloc_size attribute on both
of those if they are different types.  E.g. if somebody does

#include 

typedef void *(*allocfn) (size_t);

static inline void *
foo (allocfn fn, size_t sz)
{
   return fn (sz);
}

static inline void *
bar (size_t sz)
{
   return foo (malloc, sz);
}

then I think this patch would no longer treat it as malloc.

As this is security relevant, I'd probably look for alloc_size
attribute in both gimple_call_fntype and, if gimple_call_fndecl is non-NULL,
its TREE_TYPE.


Thanks for the test case!  I wondered if using fntype would
always work but couldn't think of when it wouldn't.  I've
adjusted the function to use both and added the test case.

While thinking about this it occurred to me that alloc_size
is only documented as a function attribute but not one that
applies to pointers or types.  I added documentation for
these uses to the Common Type and Common Variable sections.

Martin

PS Other function attributes that also apply to types and
variables are only documented in the function section.  They
should also be mentioned in the other sections.  Which, if
done in the established style, will result in duplicating
a lot of text in three places.  I think that suggests that
we might want to think about structuring these sections of
the manual differently to avoid the duplication.
PR tree-optimization/88372 - alloc_size attribute is ignored on function pointers

gcc/ChangeLog:

	PR tree-optimization/88372
	* calls.c (maybe_warn_alloc_args_overflow): Handle function pointers.
	* tree-object-size.c (alloc_object_size): Same.  Simplify.
	* doc/extend.texi (Object Size Checking): Update.
	(Other Builtins): Add __builtin_object_size.
	(Common Type Attributes): Add alloc_size.
	(Common Variable Attributes): Ditto.

gcc/testsuite/ChangeLog:

	PR tree-optimization/88372
	* gcc.dg/Walloc-size-larger-than-18.c: New test.
	* gcc.dg/builtin-object-size-19.c: Same.

Index: gcc/calls.c
===
--- gcc/calls.c	(revision 266862)
+++ gcc/calls.c	(working copy)
@@ -1342,9 +1342,10 @@ get_size_range (tree exp, tree range[2], bool allo
 /* Diagnose a call EXP to function FN decorated with attribute alloc_size
whose argument numbers given by IDX with values given by ARGS exceed
the maximum object size or cause an unsigned oveflow (wrapping) when
-   multiplied.  When ARGS[0] is null the function does nothing.  ARGS[1]
-   may be null for functions like malloc, and non-null for those like
-   calloc that are decorated with a two-argument attribute alloc_size.  */
+   multiplied.  FN is null when EXP is a call via a function pointer.
+   When ARGS[0] is null the function does nothing.  ARGS[1] may be null
+   for functions like malloc, and non-null for those like calloc that
+   are decorated with a two-argument attribute alloc_size.  */
 
 void
 maybe_warn_alloc_args_overflow (tree fn, tree exp, tree args[2], int idx[2])
@@ -1357,6 +1358,8 @@ maybe_warn_alloc_args_overflow (tree fn, tree exp,
 
   location_t loc = EXPR_LOCATION (exp);
 
+  tree fntype = fn ? TREE_TYPE (fn) : TREE_TYPE (TREE_TYPE (exp));
+  built_in_function fncode = fn ? DECL_FUNCTION_CODE (fn) : BUILT_IN_NONE;
   bool warned = false;
 
   /* Validate each argument individually.  */
@@ -1382,11 +1385,11 @@ maybe_warn_alloc_args_overflow (tree fn, tree exp,
 		 friends.
 		 Also avoid issuing the warning for calls to function named
 		 "alloca".  */
-	  if ((DECL_FUNCTION_CODE (fn) == BUILT_IN_ALLOCA
+	  if ((fncode == 

Re: Fortran patches

[Steve Kargl]

On Thu, Dec 06, 2018 at 08:02:43PM +0100, Thomas Koenig wrote:
> >>> PR fortran/88139
> >>> * dump-parse-tree.c (write_proc): Alternate return.
> >> I dissent with this patch. The introduced error is meaningless and, as
> >> mentioned by comment #3 in the PR, avoiding the ICE in dump-parse-tree
> >> is not directly the issue. The code should be rejected in parsing. In
> >> gcc-8.1 the invalid code is accepted (without an ICE) even without the
> >> -fc-prototypes flag: I haven't finished building the compiler with
> >> your changes yet to see whether that is still true afterwards, but at
> >> least the test case doesn't try this, so I strongly suspect the patch
> >> is incomplete to fix the PR.
> >   
> > Comment #3 does not contain a patch to fix the problem elsewhere.
> 
> I know :-)
> 
> > In F2003, 15.2.6 "Interoperability of procedures and procedure interfaces",
> > I cannot find a prohibition on an alternate return in a subroutine
> > interface with BIND(C).
> 
> I also does not allow this, and does not offer a valid interpretation
> of what it should mean.
> 
> If it has a meaning, it should be translatable into something prescribed
> by the standard with -fc-prototypes.
> 
> I have assigned the error to myself, so I will not forget to fix
> it before the gcc 9 release.
> 

I have asked on c.l.f.  It seems NAG rejects alternate return
mixed with bind(c).  FortranFan provided a complete testcase:

   subroutine foo(*) bind(C, name='f')
   end subroutine foo
program p
   interface
  subroutine bar(*) bind(C, name='f')
  end subroutine bar
   end interface
   call bar( *10 )
   print *, "Return following 'bar' invocation: jumping to 20"
   go to 20
10 print *, "THIS IS UNEXPECTED: Alternate return to 10 after bar"
20 continue
   stop
end program p

NAG rejects it.  Intel, PGI, and gfortran accept it.

-- 
Steve

Re: [PATCH, OpenACC] Enable GOMP_MAP_FIRSTPRIVATE_INT for OpenACC

[Julian Brown]

On Tue, 4 Dec 2018 15:27:12 +0100
Jakub Jelinek  wrote:

> On Thu, Sep 20, 2018 at 07:38:04PM -0400, Julian Brown wrote:
> > 2018-09-20  Cesar Philippidis  
> > Julian Brown  
> > 
> > gcc/
> > * omp-low.c (maybe_lookup_field_in_outer_ctx): New function.
> > (convert_to_firstprivate_int): New function.
> > (convert_from_firstprivate_int): New function.
> > (lower_omp_target): Enable GOMP_MAP_FIRSTPRIVATE_INT in
> > OpenACC.
> > 
> > libgomp/
> > * oacc-parallel.c (GOACC_parallel_keyed): Handle
> > GOMP_MAP_FIRSTPRIVATE_INT host addresses.
> > * plugin/plugin-nvptx.c (nvptx_exec): Handle
> > GOMP_MAP_FIRSTPRIVATE_INT host addresses.
> > * testsuite/libgomp.oacc-c++/firstprivate-int.C: New test.
> > * testsuite/libgomp.oacc-c-c++-common/firstprivate-int.c:
> > New test.
> > * testsuite/libgomp.oacc-fortran/firstprivate-int.f90: New
> > test.  
> 
> > @@ -8039,7 +8182,7 @@ lower_omp_target (gimple_stmt_iterator
> > *gsi_p, omp_context *ctx) if (omp_is_reference (ovar))
> >   type = TREE_TYPE (type);
> > if ((INTEGRAL_TYPE_P (type)
> > -&& TYPE_PRECISION (type) <= POINTER_SIZE)
> > +&& tree_to_uhwi (TYPE_SIZE (type)) <=
> > POINTER_SIZE) || TREE_CODE (type) == POINTER_TYPE)
> >   {
> > tkind = GOMP_MAP_FIRSTPRIVATE_INT;
> > @@ -8194,7 +8337,7 @@ lower_omp_target (gimple_stmt_iterator
> > *gsi_p, omp_context *ctx) if (omp_is_reference (var))
> >   type = TREE_TYPE (type);
> > if ((INTEGRAL_TYPE_P (type)
> > -&& TYPE_PRECISION (type) <= POINTER_SIZE)
> > +&& tree_to_uhwi (TYPE_SIZE (type)) <=
> > POINTER_SIZE) || TREE_CODE (type) == POINTER_TYPE)
> >   {
> > x = build_receiver_ref (var, false, ctx);  
> 
> Why this?

My *guess* is that it was an attempt to handle cases where the type
precision is less than the type size, and maybe it was feared that
type-punning to an int would then copy the wrong bits. Those changes
appear to not have been necessary though, at least with respect to
testsuite coverage. I also fixed the Fortran test to use "STOP n"
instead of "call abort".

I re-tested the attached with offloading to nvptx. OK?

Thanks,

Julian
commit 5c5d0e7ca29413ba8ec0c38b616a7c59f36f56cd
Author: Julian Brown 
Date:   Mon Sep 17 19:38:21 2018 -0700

Enable GOMP_MAP_FIRSTPRIVATE_INT for OpenACC

	gcc/
	* omp-low.c (maybe_lookup_field_in_outer_ctx): New function.
	(convert_to_firstprivate_int): New function.
	(convert_from_firstprivate_int): New function.
	(lower_omp_target): Enable GOMP_MAP_FIRSTPRIVATE_INT in OpenACC.

	libgomp/
	* oacc-parallel.c (GOACC_parallel_keyed): Handle
	GOMP_MAP_FIRSTPRIVATE_INT host addresses.
	* plugin/plugin-nvptx.c (nvptx_exec): Handle GOMP_MAP_FIRSTPRIVATE_INT
	host addresses.
	* testsuite/libgomp.oacc-c++/firstprivate-int.C: New test.
	* testsuite/libgomp.oacc-c-c++-common/firstprivate-int.c: New test.
	* testsuite/libgomp.oacc-fortran/firstprivate-int.f90: New test.

diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index b406ce7..4718a65 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -3497,6 +3497,19 @@ maybe_lookup_decl_in_outer_ctx (tree decl, omp_context *ctx)
   return t ? t : decl;
 }
 
+/* Returns true if DECL is present inside a field that encloses CTX.  */
+
+static bool
+maybe_lookup_field_in_outer_ctx (tree decl, omp_context *ctx)
+{
+  omp_context *up;
+
+  for (up = ctx->outer; up; up = up->outer)
+if (maybe_lookup_field (decl, up))
+  return true;
+
+  return false;
+}
 
 /* Construct the initialization value for reduction operation OP.  */
 
@@ -9052,6 +9065,88 @@ lower_omp_taskreg (gimple_stmt_iterator *gsi_p, omp_context *ctx)
 }
 }
 
+/* Helper function for lower_omp_target.  Converts VAR to something
+   that can be represented by a POINTER_SIZED_INT_NODE.  Any new
+   instructions are appended to GS.  This is primarily used to
+   optimize firstprivate variables, so that small types (less
+   precision than POINTER_SIZE) do not require additional data
+   mappings. */
+
+static tree
+convert_to_firstprivate_int (tree var, gimple_seq *gs)
+{
+  tree type = TREE_TYPE (var), new_type = NULL_TREE;
+  tree tmp = NULL_TREE;
+
+  if (omp_is_reference (var))
+type = TREE_TYPE (type);
+
+  if (INTEGRAL_TYPE_P (type) || POINTER_TYPE_P (type))
+{
+  if (omp_is_reference (var))
+	{
+	  tmp = create_tmp_var (type);
+	  gimplify_assign (tmp, build_simple_mem_ref (var), gs);
+	  var = tmp;
+	}
+
+  return fold_convert (pointer_sized_int_node, var);
+}
+
+  gcc_assert (tree_to_uhwi (TYPE_SIZE (type)) <= POINTER_SIZE);
+
+  new_type = lang_hooks.types.type_for_size (tree_to_uhwi (TYPE_SIZE (type)),
+	 true);
+
+  if (omp_is_reference (var))
+{
+  tmp = create_tmp_var (type);
+  gimplify_assign (tmp, build_simple_mem_ref (var), gs);
+  var = tmp;
+}
+
+ 

Re: [PATCH, PPC/Darwin] Fix long double symbol exports.

[Mike Stump]

On Dec 6, 2018, at 11:52 AM, Iain Sandoe  wrote:
> 
> During 8.x, the rs6000 target-specific mangling was reorganised which 
> uncovered
> a long-standing bug in Darwin’s mangling for ‘IBM’ long double.  Now the 
> symbols
> are correctly mangled, and we end up with a bunch of test link fails.
> 
> This patch adds the necessary subset of the Linux long double exports to 
> Darwin’s
> export table.
> 
> I have tested this on a few bootstrap/regtest cycles on powerpc-darwin9, and 
> on the
> power7 linux system.
> 
> For the record, I’ve noted the library versions from the Linux side, although 
> Darwin
> does not version symbols in this way.
> 
> OK for trunk and 8.x?

Don't know if the libstdc++ want to review this or they want me to...  I'm fine 
with it.

Re: [PATCH 0/6, OpenACC, libgomp] Async re-work

[Julian Brown]

On Thu, 6 Dec 2018 22:22:46 +
Julian Brown  wrote:

> On Thu, 6 Dec 2018 21:42:14 +0100
> Thomas Schwinge  wrote:
> 
> > [...]
> > ..., where the "Invalid read of size 8" happens, and which
> > eventually would try to "free (tgt)" again, via
> > libgomp/target.c:gomp_unmap_tgt:
> > 
> > attribute_hidden void
> > gomp_unmap_tgt (struct target_mem_desc *tgt)
> > {
> >   /* Deallocate on target the tgt->tgt_start .. tgt->tgt_end
> > region.  */ if (tgt->tgt_end)
> > gomp_free_device_memory (tgt->device_descr, tgt->to_free);
> > 
> >   free (tgt->array);
> >   free (tgt);
> > }
> > 
> > Is the "free (tgt)" in libgomp/target.c:gomp_unmap_vars_async wrong,
> > or something else?  
> 
> It might be worth trying this with the refcounting changes in the
> attach/detach patch.

...oh, also make sure you have this patch in the series you're testing
with:

https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01973.html

else your "wait" will be ignored, IIUC.

Julian

Re: [PATCH 0/6, OpenACC, libgomp] Async re-work

[Julian Brown]

On Thu, 6 Dec 2018 21:42:14 +0100
Thomas Schwinge  wrote:

> [...]
> ..., where the "Invalid read of size 8" happens, and which eventually
> would try to "free (tgt)" again, via libgomp/target.c:gomp_unmap_tgt:
> 
> attribute_hidden void
> gomp_unmap_tgt (struct target_mem_desc *tgt)
> {
>   /* Deallocate on target the tgt->tgt_start .. tgt->tgt_end
> region.  */ if (tgt->tgt_end)
> gomp_free_device_memory (tgt->device_descr, tgt->to_free);
> 
>   free (tgt->array);
>   free (tgt);
> }
> 
> Is the "free (tgt)" in libgomp/target.c:gomp_unmap_vars_async wrong,
> or something else?

It might be worth trying this with the refcounting changes in the
attach/detach patch.

Julian

Re: C++ patch ping

[Jason Merrill]

On 12/4/18 9:47 AM, Jakub Jelinek wrote:

Hi!

I'd like to ping
   PR87506 - https://gcc.gnu.org/ml/gcc-patches/2018-11/msg01758.html

You've acked the patch with the asserts but that FAILs as mentioned
in the above mail.  The following has been bootstrapped/regtested
and works, can it be committed without those asserts and let those
be handled incrementally later (though, I'm afraid I'm not familiar enough
with resolving those).


OK>

Jason

Re: [PATCH] handle function pointers in __builtin_object_size (PR 88372)

[Jakub Jelinek]

On Thu, Dec 06, 2018 at 01:21:58PM -0700, Martin Sebor wrote:
> Bug 88372 - alloc_size attribute is ignored on function pointers
> points out that even though the alloc_size attribute is accepted
> on function pointers it doesn't have any effect on Object Size
> Checking.  The reporter, who is implementing the feature in Clang,
> wants to know if by exposing it under the same name they won't be
> causing incompatibilities with GCC.
> 
> I don't think it's intentional that GCC doesn't take advantage of
> the attribute for Object Size Checking, and certainly not to detect
> the same kinds of issues as with other allocation functions (such
> as excessive or negative size arguments).  Rather, it's almost
> certainly an oversight since GCC does make use of function pointer
> attributes in other contexts (e.g., attributes alloc_align and
> noreturn).
> 
> As an oversight, I think it's fair to consider it a bug rather
> than a request for an enhancement.  Since not handling
> the attribute in Object Size Checking has adverse security
> implications, I also think this bug should be addressed in GCC
> 9.  With that, I submit the attached patch to resolve both
> aspects of the problem.

This is because alloc_object_size has been written before we had attributes
like alloc_size.  The only thing I'm unsure about is whether we should
prefer gimple_call_fntype or TREE_TYPE (gimple_call_fndecl ()) if it is a
direct call or if we should try to look for alloc_size attribute on both
of those if they are different types.  E.g. if somebody does

#include 

typedef void *(*allocfn) (size_t);

static inline void *
foo (allocfn fn, size_t sz)
{
  return fn (sz);
}

static inline void *
bar (size_t sz)
{
  return foo (malloc, sz);
}

then I think this patch would no longer treat it as malloc.

As this is security relevant, I'd probably look for alloc_size
attribute in both gimple_call_fntype and, if gimple_call_fndecl is non-NULL,
its TREE_TYPE.

Otherwise, the patch looks reasonable to me.

Jakub

RFC: libiberty PATCH to disable demangling of ancient mangling schemes

[Jason Merrill]

On Thu, Dec 6, 2018 at 11:14 AM Jason Merrill  wrote:
>
> Looks good to me.  Independently, do you see a reason not to disable the
> old demangler entirely?

Like so.  Does anyone object to this?  These mangling schemes haven't
been relevant in decades.
commit 175323701ad923aa47f25e1e37fa1f3c487dc5ea
Author: Jason Merrill 
Date:   Tue Nov 20 01:17:48 2018 -0500

* cplus-dem.c (cplus_demangle): Turn off the old demangler.

diff --git a/libiberty/cplus-dem.c b/libiberty/cplus-dem.c
index 4f29d54d089..8ee23b2fe71 100644
--- a/libiberty/cplus-dem.c
+++ b/libiberty/cplus-dem.c
@@ -267,6 +267,7 @@ const struct demangler_engine libiberty_demanglers[] =
   "Automatic selection based on executable"
   }
   ,
+#ifdef OLD_DEMANGLERS
   {
 GNU_DEMANGLING_STYLE_STRING,
   gnu_demangling,
@@ -297,10 +298,11 @@ const struct demangler_engine libiberty_demanglers[] =
   "EDG style demangling"
   }
   ,
+#endif
   {
 GNU_V3_DEMANGLING_STYLE_STRING,
 gnu_v3_demangling,
-"GNU (g++) V3 ABI-style demangling"
+"GNU (g++) V3 (Itanium C++ ABI) style demangling"
   }
   ,
   {
@@ -915,8 +917,12 @@ cplus_demangle (const char *mangled, int options)
 	return ret;
 }
 
+#if OLD_DEMANGLERS
+  /* People have been busily breaking the old demangler with fuzzers
+ (CVE-2018-12641 etc), so let's turn it off.  */
   ret = internal_cplus_demangle (work, mangled);
   squangle_mop_up (work);
+#endif
   return (ret);
 }
 

C++ PATCH for c++/88136, -Wdeprecated-copy too noisy

[Jason Merrill]

-Wdeprecated-copy does find some real bugs, but it also complains
about a lot of reasonable code for which the implicitly declared copy
ctor/op= are fine oven though the class has a user-defined destructor:
this situation is only problematic if the destructor releases
resources held in one of the non-static data members.

So, this patch reins it in somewhat: first by moving from -Wall to
-Wextra, and then also only complaining if the other copy op is
user-declared.  The old behavior can be explicitly requested with
-Wdeprecated-copy-dtor.

Tested x86_64-pc-linux-gnu, applying to trunk.
commit c37377416dcbf21c6b9f6c7b83fd8f9587b0a517
Author: Jason Merrill 
Date:   Wed Nov 21 18:04:02 2018 -0500

PR c++/88136 - -Wdeprecated-copy false positives

Deprecating the copy operations because the class has a user-provided
destructor turns out to have too many false positives; this patch adjusts
-Wdeprecated-copy to only deprecate if the other copy operation is
user-provided.  To get the earlier behavior, people can explicitly request
it with -Wdeprecated-copy-dtor.

gcc/c-family/
* c.opt (Wdeprecated-copy-dtor): New.
(Wdeprecated-copy): Move to -Wextra.
gcc/cp/
* class.c (classtype_has_depr_implicit_copy): Rename from
classtype_has_user_copy_or_dtor.
* method.c (lazily_declare_fn): Adjust.
* decl2.c (cp_warn_deprecated_use): Refer to -Wdeprecated-copy-dtor
if deprecation is due to a destructor.

diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 3b6912ea1cc..98c1a748329 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -228,7 +228,8 @@ in the following sections.
 -fvisibility-ms-compat @gol
 -fext-numeric-literals @gol
 -Wabi=@var{n}  -Wabi-tag  -Wconversion-null  -Wctor-dtor-privacy @gol
--Wdelete-non-virtual-dtor  -Wdeprecated-copy  -Wliteral-suffix @gol
+-Wdelete-non-virtual-dtor  -Wdeprecated-copy  -Wdeprecated-copy-dtor @gol
+-Wliteral-suffix @gol
 -Wmultiple-inheritance  -Wno-init-list-lifetime @gol
 -Wnamespaces  -Wnarrowing @gol
 -Wpessimizing-move  -Wredundant-move @gol
@@ -3000,8 +3001,10 @@ by @option{-Wall}.
 @opindex Wno-deprecated-copy
 Warn that the implicit declaration of a copy constructor or copy
 assignment operator is deprecated if the class has a user-provided
-copy constructor, copy assignment operator, or destructor, in C++11
-and up.  This warning is enabled by @option{-Wall}.
+copy constructor or copy assignment operator, in C++11 and up.  This
+warning is enabled by @option{-Wextra}.  With
+@option{-Wdeprecated-copy-dtor}, also deprecate if the class has a
+user-provided destructor.
 
 @item -Wno-init-list-lifetime @r{(C++ and Objective-C++ only)}
 @opindex Winit-list-lifetime
@@ -4407,6 +4410,7 @@ name is still supported, but the newer name is more descriptive.)
 
 @gccoptlist{-Wclobbered  @gol
 -Wcast-function-type  @gol
+-Wdeprecated-copy @r{(C++ only)} @gol
 -Wempty-body  @gol
 -Wignored-qualifiers @gol
 -Wimplicit-fallthrough=3 @gol
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 6f88a1013d6..07ff1c84f96 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -481,7 +481,12 @@ C C++ ObjC ObjC++ CPP(cpp_warn_deprecated) CppReason(CPP_W_DEPRECATED) Var(warn_
 Warn if a deprecated compiler feature, class, method, or field is used.
 
 Wdeprecated-copy
-C++ ObjC++ Var(warn_deprecated_copy) Warning LangEnabledBy(C++ ObjC++, Wall)
+C++ ObjC++ Var(warn_deprecated_copy) Warning LangEnabledBy(C++ ObjC++, Wextra)
+Mark implicitly-declared copy operations as deprecated if the class has a
+user-provided copy operation.
+
+Wdeprecated-copy-dtor
+C++ ObjC++ Var(warn_deprecated_copy, 2) Warning
 Mark implicitly-declared copy operations as deprecated if the class has a
 user-provided copy operation or destructor.
 
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 111a123bb34..886abeaa3f9 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -6272,7 +6272,7 @@ extern bool type_has_constexpr_default_constructor (tree);
 extern bool type_has_virtual_destructor		(tree);
 extern bool classtype_has_move_assign_or_move_ctor_p (tree, bool user_declared);
 extern bool classtype_has_non_deleted_move_ctor (tree);
-extern tree classtype_has_user_copy_or_dtor	(tree);
+extern tree classtype_has_depr_implicit_copy	(tree);
 extern bool type_build_ctor_call		(tree);
 extern bool type_build_dtor_call		(tree);
 extern void explain_non_literal_class		(tree);
diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index 57261511a90..9c175f85cf6 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -5233,7 +5233,7 @@ classtype_has_non_deleted_move_ctor (tree t)
operator, or destructor, returns that function.  Otherwise, null.  */
 
 tree
-classtype_has_user_copy_or_dtor (tree t)
+classtype_has_depr_implicit_copy (tree t)
 {
   if (!CLASSTYPE_LAZY_COPY_CTOR (t))
 for (ovl_iterator iter (CLASSTYPE_CONSTRUCTORS (t)); iter; ++iter)
diff --git 

RFA: PATCH to add pp command to gdbinit.in

[Jason Merrill]

Since pvt was removed, it's bugged me that to pretty-print a vec I
needed to write out "call debug($)".  So this patch adds a generic
command "pp" to print anything handled by a debug overload.

OK for trunk?
commit 2dd2501e3abbd9d0b70119534fa5a93e957432bf
Author: Jason Merrill 
Date:   Tue Nov 20 01:18:00 2018 -0500

* gdbinit.in (pp): New macro.

(pbb): Fix.

diff --git a/gcc/gdbinit.in b/gcc/gdbinit.in
index 4db977f0bab..e7d34686864 100644
--- a/gcc/gdbinit.in
+++ b/gcc/gdbinit.in
@@ -16,6 +16,15 @@
 # along with GCC; see the file COPYING3.  If not see
 # .
 
+define pp
+call debug ($)
+end
+
+document pp
+Print a representation of the GCC data structure that is $.
+Works only when an inferior is executing.
+end
+
 define pr
 set debug_rtx ($)
 end
@@ -167,7 +176,7 @@ including the global binding level.
 end
 
 define pbb
-set debug ($)
+call debug ($)
 end
 
 document pbb

Re: [PATCH 6/6, OpenACC, libgomp] Async re-work, nvptx changes

[Thomas Schwinge]

Hi Chung-Lin!

On Tue, 25 Sep 2018 21:11:58 +0800, Chung-Lin Tang  
wrote:
> Hi Tom,
> this patch removes large portions of plugin/plugin-nvptx.c, since a lot of it 
> is
> now in oacc-async.c now. The new code is essentially a NVPTX/CUDA-specific 
> implementation
> of the new-style goacc_asyncqueues.

> --- a/libgomp/plugin/plugin-nvptx.c
> +++ b/libgomp/plugin/plugin-nvptx.c

> +struct goacc_asyncqueue *
> +GOMP_OFFLOAD_openacc_async_construct (void)
> +{
> +  struct goacc_asyncqueue *aq
> += GOMP_PLUGIN_malloc (sizeof (struct goacc_asyncqueue));
> +  aq->cuda_stream = NULL;
> +  CUDA_CALL_ASSERT (cuStreamCreate, >cuda_stream, CU_STREAM_DEFAULT);

Curiously (this was the same in the code before): does this have to be
"CU_STREAM_DEFAULT" instead of "CU_STREAM_NON_BLOCKING", because we want
to block anything from running in parallel with "acc_async_sync" GPU
kernels, that use the "NULL" stream?  (Not asking you to change this now,
but I wonder if this is overly strict?)

> +  if (aq->cuda_stream == NULL)
> +GOMP_PLUGIN_fatal ("CUDA stream create NULL\n");

Can this actually happen, given the "CUDA_CALL_ASSERT" usage above?

> +  CUDA_CALL_ASSERT (cuStreamSynchronize, aq->cuda_stream);

Why is the synchronization needed here?

> +  return aq;
> +}


Grüße
 Thomas

Re: [PATCH 0/6, OpenACC, libgomp] Async re-work

[Thomas Schwinge]

Hi Chung-Lin!

On Tue, 25 Sep 2018 21:09:49 +0800, Chung-Lin Tang  
wrote:
> This patch is a re-organization of OpenACC asynchronous queues.

Thanks!

> The previous style of implementation
> was essentially re-defining the entire async API inside the plugin-interface, 
> and relaying all such
> API calls to the target plugin, which is awkward in design; it requires 
> (each) target plugin to
> essentially re-implement large portions of the async functionality to support 
> OpenACC, and the
> way it uses a state-setting style to "select/de-select" asynchronous queues 
> for operations litters
> a lot of code paths.
> 
> The new design proposed here in this patch declares a "struct 
> goacc_asyncqueue*" opaque type in libgomp.h,
> and re-defines the plugin interface to a few operations (e.g. 
> construct/destruct/test/synchronize/etc.)
> on this async-queue type, all details are target-dependent inside the 
> specific plugin/plugin-.c file.

Conceptually, ACK.


> Also included in this patch is the code for the acc_get/set_default_async API 
> functions in OpenACC 2.5.
> It's a minor part of this patch, but since some code was merge together, I'm 
> submitting it together here.

As I requested, I'm reviewing those changes separately, and have backed
out those changes in my working copy.


> Testing has been done with offloading enabled. The results are mostly okay, 
> but with a few issues
> with either yet incomplete submission of our testsuite adjustment patches, or 
> other independent problems.

We'll need to understand these.  


> Seeking permission to commit this to trunk first.

A few things will need to be clarified.


For example, for the simple program:

int main(void)
{
#pragma acc parallel async(1)
  ;
#pragma acc wait

  return 0;
}

..., I'm seeing memory corruption, which (oaccasionally...) shows up as
an abort due to "free" complaining, but also reproduces more reliably
with "valgrind".  It also reproduces on openacc-gcc-8-branch:

$ valgrind ./a.out
[...]
==26392== Invalid read of size 8
==26392==at 0x4E653B0: goacc_async_unmap_tgt (oacc-async.c:368)
==26392==by 0x5C90901: cuda_callback_wrapper (plugin-nvptx.c:1648)
==26392==by 0x6066B8D: ??? (in 
/usr/lib/x86_64-linux-gnu/libcuda.so.390.77)
==26392==by 0x607A10F: ??? (in 
/usr/lib/x86_64-linux-gnu/libcuda.so.390.77)
==26392==by 0x50816DA: start_thread (pthread_create.c:463)
==26392==by 0x53BA88E: clone (clone.S:95)
==26392==  Address 0x8d19f50 is 0 bytes inside a block of size 64 free'd
==26392==at 0x4C30D3B: free (vg_replace_malloc.c:530)
==26392==by 0x4E65BEE: goacc_async_copyout_unmap_vars (oacc-async.c:383)
==26392==by 0x4E607C9: GOACC_parallel_keyed_internal 
(oacc-parallel.c:403)
==26392==by 0x4E60EAA: GOACC_parallel_keyed_v2 (oacc-parallel.c:439)
==26392==by 0x40094F: ??? (in [...]/a.out)
==26392==by 0x52BAB96: (below main) (libc-start.c:310)
==26392==  Block was alloc'd at
==26392==at 0x4C2FB0F: malloc (vg_replace_malloc.c:299)
==26392==by 0x4E47538: gomp_malloc (alloc.c:37)
==26392==by 0x4E5AEEB: gomp_map_vars_async (target.c:731)
==26392==by 0x4E60C2B: GOACC_parallel_keyed_internal 
(oacc-parallel.c:345)
==26392==by 0x4E60EAA: GOACC_parallel_keyed_v2 (oacc-parallel.c:439)
==26392==by 0x40094F: ??? (in [...]/a.out)
==26392==by 0x52BAB96: (below main) (libc-start.c:310)
[...]

Per my understanding, the problem is that, called from
libgomp/oacc-async.c:goacc_async_copyout_unmap_vars,
libgomp/target.c:gomp_unmap_vars_async runs into:

  if (tgt->list_count == 0)
{
  free (tgt);
  return;
}

..., and then goacc_async_copyout_unmap_vars does:

  devicep->openacc.async.queue_callback_func (aq, goacc_async_unmap_tgt,
  (void *) tgt);

..., which will then call libgomp/oacc-async.c:goacc_async_unmap_tgt:

static void
goacc_async_unmap_tgt (void *ptr)
{
  struct target_mem_desc *tgt = (struct target_mem_desc *) ptr;

  if (tgt->refcount > 1)
tgt->refcount--;
  else
gomp_unmap_tgt (tgt);
}

..., where the "Invalid read of size 8" happens, and which eventually
would try to "free (tgt)" again, via libgomp/target.c:gomp_unmap_tgt:

attribute_hidden void
gomp_unmap_tgt (struct target_mem_desc *tgt)
{
  /* Deallocate on target the tgt->tgt_start .. tgt->tgt_end region.  */
  if (tgt->tgt_end)
gomp_free_device_memory (tgt->device_descr, tgt->to_free);

  free (tgt->array);
  free (tgt);
}

Is the "free (tgt)" in libgomp/target.c:gomp_unmap_vars_async wrong, or
something else?


Grüße
 Thomas

Re: [PATCH] Fix VRP with -fno-delete-null-pointer-checks (PR c/88367)

[Jakub Jelinek]

On Thu, Dec 06, 2018 at 01:08:34PM -0700, Jeff Law wrote:
> > I hope we can still say that pointer wrapping even with
> > -fno-delete-null-pointer-checks is UB, so this patch differentiates between
> > positive offsets (in ssizetype), negative offsets (in ssizetype) and zero
> > offsets and handles both the same for both ptr p+ offset and _REF[ptr, 
> > offset]
> > If offset is 0 and ptr is ~[0, 0], then the result is ~[0, 0] as before.
> > If offset is positive in ssizetype, then even for VARYING ptr the result is
> > ~[0, 0] pointer.  If the offset is (or maybe could be) negative in
> > ssizetype, then for -fno-delete-null-pointer-checks the result is VARYING,
> > as we could go from a non-NULL pointer back to NULL on those targets; for
> > -fdelete-null-pointer-checks we do what we've done before, i.e. ~[0, 0].
> I'm not sure why we'd treat subtraction and addition any differently,
> but maybe I'm missing something subtle (or not so subtle).
> 
> ISTM that ~[0,0] +-  still results in ~[0,0] for
> -fdelete-null-pointer-checks.

Yes, the patch preserves that (unless -fwrapv-pointers).
Additionally, it does VARYING += ~[0,0] in that mode to ~[0,0].

>  For -fno-delete-null-pointer-checks ISTM
> we should indicate "we don't know anything about the result" of such an
> operation.

There are cases where we still know something.  The largest valid object
that can be supported is half of the address space, so without pointer
wrapping, positive additions to the pointer shouldn't wrap around and yield
NULL, negative ones can.  With -fwrapv-pointers anything can happen, sure,
the only case handled in that case is &[ptr + 0] if ptr is ~[0, 0] then
&[ptr + 0] is also ~[0, 0].

Jakub

[PATCH] handle function pointers in __builtin_object_size (PR 88372)

[Martin Sebor]

Bug 88372 - alloc_size attribute is ignored on function pointers
points out that even though the alloc_size attribute is accepted
on function pointers it doesn't have any effect on Object Size
Checking.  The reporter, who is implementing the feature in Clang,
wants to know if by exposing it under the same name they won't be
causing incompatibilities with GCC.

I don't think it's intentional that GCC doesn't take advantage of
the attribute for Object Size Checking, and certainly not to detect
the same kinds of issues as with other allocation functions (such
as excessive or negative size arguments).  Rather, it's almost
certainly an oversight since GCC does make use of function pointer
attributes in other contexts (e.g., attributes alloc_align and
noreturn).

As an oversight, I think it's fair to consider it a bug rather
than a request for an enhancement.  Since not handling
the attribute in Object Size Checking has adverse security
implications, I also think this bug should be addressed in GCC
9.  With that, I submit the attached patch to resolve both
aspects of the problem.

Tested on x86_64-redhat-linux.

Martin
PR tree-optimization/88372 - alloc_size attribute is ignored on function pointers

gcc/ChangeLog:

	PR tree-optimization/88372
	* calls.c (maybe_warn_alloc_args_overflow): Handle function pointers.
	* tree-object-size.c (alloc_object_size): Same.  Simplify.
	* doc/extend.texi (Object Size Checking): Update.
	(Other Builtins): Add __builtin_object_size.

gcc/testsuite/ChangeLog:

	PR tree-optimization/88372
	* gcc.dg/Walloc-size-larger-than-18.c: New test.
	* gcc.dg/builtin-object-size-19.c: Same.

Index: gcc/calls.c
===
--- gcc/calls.c	(revision 266862)
+++ gcc/calls.c	(working copy)
@@ -1342,9 +1342,10 @@ get_size_range (tree exp, tree range[2], bool allo
 /* Diagnose a call EXP to function FN decorated with attribute alloc_size
whose argument numbers given by IDX with values given by ARGS exceed
the maximum object size or cause an unsigned oveflow (wrapping) when
-   multiplied.  When ARGS[0] is null the function does nothing.  ARGS[1]
-   may be null for functions like malloc, and non-null for those like
-   calloc that are decorated with a two-argument attribute alloc_size.  */
+   multiplied.  FN is null when EXP is a call via a function pointer.
+   When ARGS[0] is null the function does nothing.  ARGS[1] may be null
+   for functions like malloc, and non-null for those like calloc that
+   are decorated with a two-argument attribute alloc_size.  */
 
 void
 maybe_warn_alloc_args_overflow (tree fn, tree exp, tree args[2], int idx[2])
@@ -1357,6 +1358,8 @@ maybe_warn_alloc_args_overflow (tree fn, tree exp,
 
   location_t loc = EXPR_LOCATION (exp);
 
+  tree fntype = fn ? TREE_TYPE (fn) : TREE_TYPE (TREE_TYPE (exp));
+  built_in_function fncode = fn ? DECL_FUNCTION_CODE (fn) : BUILT_IN_NONE;
   bool warned = false;
 
   /* Validate each argument individually.  */
@@ -1382,11 +1385,11 @@ maybe_warn_alloc_args_overflow (tree fn, tree exp,
 		 friends.
 		 Also avoid issuing the warning for calls to function named
 		 "alloca".  */
-	  if ((DECL_FUNCTION_CODE (fn) == BUILT_IN_ALLOCA
+	  if ((fncode == BUILT_IN_ALLOCA
 		   && IDENTIFIER_LENGTH (DECL_NAME (fn)) != 6)
-		  || (DECL_FUNCTION_CODE (fn) != BUILT_IN_ALLOCA
+		  || (fncode != BUILT_IN_ALLOCA
 		  && !lookup_attribute ("returns_nonnull",
-	TYPE_ATTRIBUTES (TREE_TYPE (fn)
+	TYPE_ATTRIBUTES (fntype
 		warned = warning_at (loc, OPT_Walloc_zero,
  "%Kargument %i value is zero",
  exp, idx[i] + 1);
@@ -1398,6 +1401,7 @@ maybe_warn_alloc_args_overflow (tree fn, tree exp,
 		 size overflow.  There's no good way to detect C++98 here
 		 so avoid diagnosing these calls for all C++ modes.  */
 	  if (i == 0
+		  && fn
 		  && !args[1]
 		  && lang_GNU_CXX ()
 		  && DECL_IS_OPERATOR_NEW (fn)
@@ -1481,7 +1485,7 @@ maybe_warn_alloc_args_overflow (tree fn, tree exp,
 	}
 }
 
-  if (warned)
+  if (warned && fn)
 {
   location_t fnloc = DECL_SOURCE_LOCATION (fn);
 
@@ -1933,14 +1937,13 @@ initialize_argument_information (int num_actuals A
 
   bitmap_obstack_release (NULL);
 
-  /* Extract attribute alloc_size and if set, store the indices of
- the corresponding arguments in ALLOC_IDX, and then the actual
- argument(s) at those indices in ALLOC_ARGS.  */
+  /* Extract attribute alloc_size from the type of the called expression
+ (which could be a function or a function pointer) and if set, store
+ the indices of the corresponding arguments in ALLOC_IDX, and then
+ the actual argument(s) at those indices in ALLOC_ARGS.  */
   int alloc_idx[2] = { -1, -1 };
-  if (tree alloc_size
-  = (fndecl ? lookup_attribute ("alloc_size",
-TYPE_ATTRIBUTES (TREE_TYPE (fndecl)))
-	 : NULL_TREE))
+  if (tree alloc_size = lookup_attribute ("alloc_size",
+	  TYPE_ATTRIBUTES (fntype)))
 {
 

[PATCH] Testcase for PR 88297 and minor fixes

[Michael Ploujnikov]

Thanks to Martin we now have a test that exercises (cp) cloning
machinery during the WPA stage of LTO.

Also, during debugging I found that print_all_lattices would trigger
an assert if I tried to call it inside decide_whether_version_node.

Finally I've attached some comment spelling fixes as a bonus.


Bootstrapping (--with-build-config=bootstrap-lto) and regression testing on 
x86_64.

Ok for trunk after tests pass?


- Michael
From f8f59d44141726e688cde077aabb5f2ce0bf53e0 Mon Sep 17 00:00:00 2001
From: Michael Ploujnikov 
Date: Thu, 6 Dec 2018 13:36:51 -0500
Subject: [PATCH 1/3] Skip constprop clones because we don't make lattices for
 them.

gcc/ChangeLog:

2018-12-06  Michael Ploujnikov  

	* ipa-cp.c (print_all_lattices): Skip cp clones.
---
 gcc/ipa-cp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git gcc/ipa-cp.c gcc/ipa-cp.c
index c7c462ab81..c4e879bbc6 100644
--- gcc/ipa-cp.c
+++ gcc/ipa-cp.c
@@ -542,6 +542,9 @@ print_all_lattices (FILE * f, bool dump_sources, bool dump_benefits)
   struct ipa_node_params *info;
 
   info = IPA_NODE_REF (node);
+  /* Skip constprop clones since we don't make lattices for them.  */
+  if (info->ipcp_orig_node)
+	continue;
   fprintf (f, "  Node: %s:\n", node->dump_name ());
   count = ipa_get_param_count (info);
   for (i = 0; i < count; i++)
-- 
2.19.1

From 1ec489bbb30f410144c0bc84f0c160a8fbed0be6 Mon Sep 17 00:00:00 2001
From: Michael Ploujnikov 
Date: Thu, 6 Dec 2018 13:47:00 -0500
Subject: [PATCH 2/3] Testcase for cloning during LTO WPA stage.

Written with Martin Jambor's help:
https://gcc.gnu.org/ml/gcc/2018-12/msg00043.html

gcc/testsuite/ChangeLog:

2018-12-06  Michael Ploujnikov  

	* gcc.dg/lto/pr88297_0.c: New test.
	* gcc.dg/lto/pr88297_1.c: New test.
---
 gcc/testsuite/gcc.dg/lto/pr88297_0.c | 57 
 gcc/testsuite/gcc.dg/lto/pr88297_1.c | 25 
 2 files changed, 82 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/lto/pr88297_0.c
 create mode 100644 gcc/testsuite/gcc.dg/lto/pr88297_1.c

diff --git gcc/testsuite/gcc.dg/lto/pr88297_0.c gcc/testsuite/gcc.dg/lto/pr88297_0.c
new file mode 100644
index 00..d415015166
--- /dev/null
+++ gcc/testsuite/gcc.dg/lto/pr88297_0.c
@@ -0,0 +1,57 @@
+/* { dg-require-effective-target lto } */
+/* { dg-lto-options {{-flto -O3 -fipa-cp -fipa-cp-clone}}  } */
+/* { dg-lto-do run } */
+
+/* In order to trigger IPA-CP cloning we have to:
+
+  1. Put the calls in main into a loop; otherwise everything is
+  coldand we would not clone.
+
+  2. Make different foos and bars actually semantically different;
+  otherwise IPA-ICF unified them (as it should).
+
+*/
+
+volatile int g;
+
+void __attribute__ ((noipa))
+use (int v)
+{
+  g = v;
+}
+
+static int __attribute__ ((noinline))
+foo (int arg)
+{
+  return 7 * arg;
+}
+
+static int __attribute__ ((noinline))
+bar (int arg)
+{
+  return arg * arg;
+}
+
+extern int __attribute__ ((noinline))
+entry2 (void);
+
+int  __attribute__ ((noipa))
+get_opaque_number (void)
+{
+  return 1;
+}
+
+int main (void)
+{
+  int i;
+  for (i = 0; i < get_opaque_number (); i++)
+{
+  use (bar (3));
+  use (bar (4));
+  use (foo (5));
+  use (foo (6));
+
+  entry2 ();
+}
+  return 0;
+}
diff --git gcc/testsuite/gcc.dg/lto/pr88297_1.c gcc/testsuite/gcc.dg/lto/pr88297_1.c
new file mode 100644
index 00..65c5321cde
--- /dev/null
+++ gcc/testsuite/gcc.dg/lto/pr88297_1.c
@@ -0,0 +1,25 @@
+extern void __attribute__ ((noipa))
+use (int v);
+
+
+static int __attribute__ ((noinline))
+foo (int arg)
+{
+  return 8 * arg;
+}
+
+static int __attribute__ ((noinline))
+bar (int arg)
+{
+  return arg * arg + 3;
+}
+
+int __attribute__ ((noinline))
+entry2 (void)
+{
+  use (bar (3));
+  use (bar (4));
+  use (foo (5));
+  use (foo (6));
+  return 0;
+}
-- 
2.19.1

From c2db1a6aa7d6787685a28df1e677a66bac6cb9b5 Mon Sep 17 00:00:00 2001
From: Michael Ploujnikov 
Date: Thu, 6 Dec 2018 14:06:55 -0500
Subject: [PATCH 3/3] Spelling fixes in comments.

gcc/ChangeLog:

2018-12-06  Michael Ploujnikov  

	* ipa-cp.c (ipa_get_indirect_edge_target_1): Fix spelling in
	comments.
---
 gcc/ipa-cp.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git gcc/ipa-cp.c gcc/ipa-cp.c
index c4e879bbc6..8c419e1f53 100644
--- gcc/ipa-cp.c
+++ gcc/ipa-cp.c
@@ -191,7 +191,7 @@ public:
   /* Depth first search number and low link for topological sorting of
  values.  */
   int dfs, low_link;
-  /* True if this valye is currently on the topo-sort stack.  */
+  /* True if this value is currently on the topo-sort stack.  */
   bool on_stack;
 
   ipcp_value()
@@ -883,7 +883,7 @@ ipcp_lattice::set_contains_variable ()
   return ret;
 }
 
-/* Set all aggegate lattices in PLATS to bottom and return true if they were
+/* Set all aggregate lattices in PLATS to bottom and return true if they were
not previously set as such.  */
 
 static inline bool
@@ -894,7 +894,7 @@ set_agg_lats_to_bottom 

Re: [PATCH] Fix VRP with -fno-delete-null-pointer-checks (PR c/88367)

[Jeff Law]

On 12/5/18 11:45 PM, Jakub Jelinek wrote:
> Hi!
> 
> If we consider -fno-delete-null-pointer-checks as a way to support e.g. AVR
> and other targets which can validly place objects at NULL rather than a way
> to workaround UBs in code, I believe the following testcase must pass if
> there is e.g.
Well, the intent was to be able to turn off the assumption that *0 would
cause a fault and halt the program.  That assumption allows us to use an
earlier pointer dereference to infer it currently has a non-NULL value
and eliminate subsequent tests against NULL.

It was never really meant to be a general escape hatch to allow other
forms of undefined behavior.

Though the name is particularly bad since it implies we never delete any
null pointer checks.


> 
> I hope we can still say that pointer wrapping even with
> -fno-delete-null-pointer-checks is UB, so this patch differentiates between
> positive offsets (in ssizetype), negative offsets (in ssizetype) and zero
> offsets and handles both the same for both ptr p+ offset and _REF[ptr, 
> offset]
> If offset is 0 and ptr is ~[0, 0], then the result is ~[0, 0] as before.
> If offset is positive in ssizetype, then even for VARYING ptr the result is
> ~[0, 0] pointer.  If the offset is (or maybe could be) negative in
> ssizetype, then for -fno-delete-null-pointer-checks the result is VARYING,
> as we could go from a non-NULL pointer back to NULL on those targets; for
> -fdelete-null-pointer-checks we do what we've done before, i.e. ~[0, 0].
I'm not sure why we'd treat subtraction and addition any differently,
but maybe I'm missing something subtle (or not so subtle).

ISTM that ~[0,0] +-  still results in ~[0,0] for
-fdelete-null-pointer-checks.  For -fno-delete-null-pointer-checks ISTM
we should indicate "we don't know anything about the result" of such an
operation.


Jeff

[PATCH, PPC/Darwin] Fix long double symbol exports.

[Iain Sandoe]

Hi,

During 8.x, the rs6000 target-specific mangling was reorganised which uncovered
a long-standing bug in Darwin’s mangling for ‘IBM’ long double.  Now the symbols
are correctly mangled, and we end up with a bunch of test link fails.

This patch adds the necessary subset of the Linux long double exports to 
Darwin’s
export table.

I have tested this on a few bootstrap/regtest cycles on powerpc-darwin9, and on 
the
power7 linux system.

For the record, I’ve noted the library versions from the Linux side, although 
Darwin
does not version symbols in this way.

OK for trunk and 8.x?
thanks
Iain

libstdc++/

* /config/os/bsd/darwin/ppc-extra.ver: Append long double symbols.

diff --git a/libstdc++-v3/config/os/bsd/darwin/ppc-extra.ver 
b/libstdc++-v3/config/os/bsd/darwin/ppc-extra.ver
index ffe32b6..f0aee9e 100644
--- a/libstdc++-v3/config/os/bsd/darwin/ppc-extra.ver
+++ b/libstdc++-v3/config/os/bsd/darwin/ppc-extra.ver
@@ -1 +1,22 @@
   __eprintf;
+# 3.4
+  _ZNSt14numeric_limitsIgE*;
+  _ZNSirsERg;
+  _ZNSolsEg;
+  _ZNSt13basic_istreamIwSt11char_traitsIwEErsERg;
+  _ZNSt13basic_ostreamIwSt11char_traitsIwEElsEg;
+  _ZSt14__convert_to_vIgEvPKcRT_RSt12_Ios_IostateRKP*;
+  
_ZStlsIg[cw]St11char_traitsI[cw]EERSt13basic_ostreamIT0_T1_ES6_RKSt7complexIT_E;
+  
_ZStrsIg[cw]St11char_traitsI[cw]EERSt13basic_istreamIT0_T1_ES6_RSt7complexIT_E;
+# 3.4.7
+  _ZNSi10_M_extractIgEERSiRT_;
+  _ZNSt13basic_istreamIwSt11char_traitsIwEE10_M_extractIgEERS2_RT_;
+  _ZNSo9_M_insertIgEERSoT_;
+  _ZNSt13basic_ostreamIwSt11char_traitsIwEE9_M_insertIgEERS2_T_;
+# 3.4.10
+  _ZNKSt3tr14hashIgEclEg;
+  _ZNKSt4hashIgEclEg;
+# ldbl 1.3
+  _ZT[IS]g;
+  _ZT[IS]Pg;
+  _ZT[IS]PKg;

Re: Fortran patches

[Steve Kargl]

On Thu, Dec 06, 2018 at 02:08:54PM -0500, Fritz Reese wrote:
> On Wed, Dec 5, 2018 at 7:03 PM Steve Kargl
>  wrote:
> >
> > On Wed, Dec 05, 2018 at 04:48:28PM -0500, Fritz Reese wrote:
> [...]
> > > RE:
> > > > PR fortran/88228
> > > > * expr.c (check_null, check_elemental): Work around -fdec and
> > > > initialization with logical operators operating on integers.
> > >
> > > I plan to review this section of the patch later today -- though the
> > > patch hides the segfault from the PR, I need more time to determine
> > > whether it is correct and complete.
> >
> > By the time the gfc_expr is given to check_check and check_elemental,
> > it has been reduced to a EXPR_CONSTANT, which neither routine expected.
> > I simply return early in that case.
> 
> It appears the correct solution is simply the following patch:
> 
> diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
> index b2090218d48..775a5c52c65 100644
> --- a/gcc/fortran/resolve.c
> +++ b/gcc/fortran/resolve.c
> @@ -4004,7 +4004,7 @@ resolve_operator (gfc_expr *e)
>   if (op2->ts.type != e->ts.type || op2->ts.kind != e->ts.kind)
> gfc_convert_type (op2, >ts, 1);
>   e = logical_to_bitwise (e);
> - return resolve_function (e);
> + break;
> }
> 
>sprintf (msg, _("Operands of logical operator %%<%s%%> at %%L
> are %s/%s"),
> @@ -4020,7 +4020,7 @@ resolve_operator (gfc_expr *e)
>   e->ts.type = BT_INTEGER;
>   e->ts.kind = op1->ts.kind;
>   e = logical_to_bitwise (e);
> - return resolve_function (e);
> + break;
> }

Intersting.  I wonder why resolve_function() appears here.  
Hmmm, 'svn annotate' points to r241535 with the committer
being foreese. :-)  Log message says you are trying to convert
logical op on integers for DEC.  Were trying to catch the
logical functions like NOT()?

I'll include the above in my patch Saturday activities.


> > > >PR fortran/88139
> > > >* dump-parse-tree.c (write_proc): Alternate return.
> > > I dissent with this patch. The introduced error is meaningless and, as
> > > mentioned by comment #3 in the PR, avoiding the ICE in dump-parse-tree
> > > is not directly the issue. The code should be rejected in parsing. In
> > > gcc-8.1 the invalid code is accepted (without an ICE) even without the
> > > -fc-prototypes flag: I haven't finished building the compiler with
> > > your changes yet to see whether that is still true afterwards, but at
> > > least the test case doesn't try this, so I strongly suspect the patch
> > > is incomplete to fix the PR.
> >
> > Comment #3 does not contain a patch to fix the problem elsewhere.
> >
> > In F2003, 15.2.6 "Interoperability of procedures and procedure interfaces",
> > I cannot find a prohibition on an alternate return in a subroutine
> > interface with BIND(C).
> >
> > I'm disinclined to let a patch fester in bugzilla to only attain
> > the same fate as my patch to PR68544.
> 
> According to F2008 §15.3.7.2(5):
> 
> > any dummy argument without the VALUE attribute [...] is interoperable with 
> > an entity of the
> > referenced type (ISO/IEC 9899:1999, 6.2.5, 7.17, and 7.18.1) of the formal 
> > parameter

Yep.

> Regardless of whether or not we accept alternate returns in BIND(C)
> procedures, the compiler must be at least consistent: if we accept
> them (which gfortran currently does), then we should be able to dump
> the C prototype (with -fc-prototypes), providing a formal parameter
> interoperable with the type of the alternate return dummy argument; if
> we reject them, then we should issue the error in parsing (before
> handling by -fc-prototypes). In either case, the error message should
> not be obscure or meaningless. Even so, the patch here is inconsistent
> since we accept the code, but issue an error when attempting to dump

I think we should determine what other compilers do with
BIND(C) and alternate return dummy arguments, and follow
suit.  


> > > >PR fortran/88205
> > > >* io.c (gfc_match_open): STATUS must be CHARACTER type.
> [...]
> > If I used e->where one gets
> >
> > a.f90:2:32:
> >
> > 2 |character(3), parameter :: a(0) = [character(3)::]
> >   |   1
> > Error: FORMAT tag at (1) cannot be a zero-sized array
> >
> > Now, imagine a few hundred lines separating the two statements.
> > I think the latter error locus is preferable.
> 
> Yes, I agree.
> 
> Swapping gfc_current_locus definitely works, but is possibly less
> readable(+maintainable) than my other suggestion of passing loc down
> as an argument... But that suggestion touches more code, so there are
> merits to either approach. In either case I have no real issue with
> this part of the patch regardless of implementation of the locus
> workaround.

I agree that this could get messy, but I'm hoping only
format strings need this special handling.  A Fortran
string must contain '(' and ')', so 

Re: C++ PATCH for c++/88373, wrong parse error with ~

[Jason Merrill]

On 12/6/18 11:33 AM, Marek Polacek wrote:

This patch fixes a bogus parse error with ~ in a template-argument-list.  We 
have

   S>

and cp_parser_template_argument just tries to parse each argument as a type,
id-expression, etc to see what sticks.  When it sees ~value, it tries to parse
it using cp_parser_class_name (because of the ~), which ends up calling
cp_parser_lookup_name, looking for "value", but finds nothing.  Since it's an
unqualified-id followed by <, we treat "~value" as a template name
function.  It isn't followed by "(args)", so we simulate parse error.  As a
consequence of this error, the parsing of the outermost template-id S fails.

The problem is that when we're looking up the name in cp_parser_class_name,
tag_type is typename_type, which means bindings that do not refer to types
are ignored, so the variable template "value" isn't found and we're toast.

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


OK.

Jason

Re: [C++ Patch] Three additional bitfield diagnostic tweaks (a regression fix included)

[Jason Merrill]

On 12/6/18 12:23 PM, Paolo Carlini wrote:

Hi,

On 06/12/18 16:11, Jason Merrill wrote:
2- Unfortunately I have to fix another buglet I recently introduced, 
completely similar to c++/88222 fixed by Marek. Well, at least we will 
not print anymore an empty '' when the unqualified_id is null because 
the field is unnamed.



-    error_at (declarator->id_loc,
-  "%qE is neither function nor member function; "
-  "cannot be declared friend", unqualified_id);
+    if (unqualified_id && declarator)
+  error_at (declarator->id_loc,
+    "%qE is neither function nor member function; "
+    "cannot be declared friend", unqualified_id);
+    else
+  error ("unnamed field is neither function nor member "
+ "function; cannot be declared friend");


I wonder if we want to use the 'name' variable here.


Well, the name variable doesn't seem that useful here because for the 
new testcase it has that famous catch all value "type name" .


I have been thinking that here and in other places we could imagine 
keeping only the declarator check and dropping the "name" check. 
Probably it would work. But in *many* existing places we actually check 
*only* the name thus I'm nervous about attempting that now...




3- In the non-static case too, when from grokdeclarator we are 
calling FIELD_DECL and passing the location as first argument, I 
think we want to likewise pass declarator->id_loc when available.



-    decl = build_decl (input_location,
+    decl = build_decl (declarator
+   ? declarator->id_loc
+   : input_location,


I think we want to put this in a local variable, to share with the 
static case and probably other places in grokdeclarator.


In the below I'm sharing it only with the static case, straightforward. 
Moving it one level up doesn't seem that useful because we only have 
rather safe IMHO unconditional uses either of input_location or of 
declarator->id_loc at the moment... Again, I'm pretty sure there is room 
for further clean-ups in this area, but, for 9, I'd rather take care of 
a bunch of additional small issues which I already have in my TODO list, 
in grokbitfield, for example, as already mentioned. By the way, if isn't 
already clear, I have been changing location bits only when I already 
have a set of testcases, constructed from our testsuite via (lenghty ;) 
instrumented runs.


New version of the patch attached.


OK.

Jason

[PATCH, committed] Fix PR libstdc++/64883

[Iain Sandoe]

Hi

I applied the following as discussed with Jonathan on the PR and IRC,
will back port to 8.x and 7.x in due course.



Some of Darwin's headers use always_inline so don't test that

  Because Darwin system headers use always_inline rather than
__always_inline__ the libstdc++ test will fail, even if our headers only
use the reserved form of the attribute. Don't test it on Darwin, and
assume that testing on other targets will catch any accidental misuses
in libstdc++ headers.

PR libstdc++/64883
* testsuite/17_intro/headers/c++1998/all_attributes.cc: Don't test
always_inline on Darwin.
* testsuite/17_intro/headers/c++2011/all_attributes.cc: Likewise.
* testsuite/17_intro/headers/c++2014/all_attributes.cc: Likewise.
* testsuite/17_intro/headers/c++2017/all_attributes.cc: Likewise.
* testsuite/17_intro/headers/c++2020/all_attributes.cc: Likewise.

diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
index 7e8df9db11d..74ef0b3ef06 100644
--- a/libstdc++-v3/ChangeLog
+++ b/libstdc++-v3/ChangeLog
@@ -1,5 +1,13 @@
+2018-12-06  Jonathan Wakely  
+   Iain Sandoe  

+   PR libstdc++/64883
+   * testsuite/17_intro/headers/c++1998/all_attributes.cc: Don't test
+   always_inline on Darwin.
+   * testsuite/17_intro/headers/c++2011/all_attributes.cc: Likewise.
+   * testsuite/17_intro/headers/c++2014/all_attributes.cc: Likewise.
+   * testsuite/17_intro/headers/c++2017/all_attributes.cc: Likewise.
+   * testsuite/17_intro/headers/c++2020/all_attributes.cc: Likewise.
+
PR libstdc++/88084 - Implement LWG 2777
* include/std/string_view (basic_string_view::copy): Use traits to
copy.
diff --git a/libstdc++-v3/testsuite/17_intro/headers/c++1998/all_attributes.cc 
b/libstdc++-v3/testsuite/17_intro/headers/c++1998/all_attributes.cc
index b6ff8c47d43..0e7dcf736f2 100644
--- a/libstdc++-v3/testsuite/17_intro/headers/c++1998/all_attributes.cc
+++ b/libstdc++-v3/testsuite/17_intro/headers/c++1998/all_attributes.cc
@@ -21,9 +21,9 @@
 // Ensure the library only uses the __name__ form for attributes.
 // Don't test 'const' because it is reserved anyway.
 #define abi_tag 1
-#define always_inline 1
 #ifndef __APPLE__
 // darwin headers use these, see PR 64883
+# define always_inline 1
 # define deprecated 1
 # define noreturn 1
 # define visibility 1
diff --git a/libstdc++-v3/testsuite/17_intro/headers/c++2011/all_attributes.cc 
b/libstdc++-v3/testsuite/17_intro/headers/c++2011/all_attributes.cc
index 33b759f7399..82f372d8294 100644
--- a/libstdc++-v3/testsuite/17_intro/headers/c++2011/all_attributes.cc
+++ b/libstdc++-v3/testsuite/17_intro/headers/c++2011/all_attributes.cc
@@ -21,11 +21,11 @@
 // Ensure the library only uses the __name__ form for attributes.
 // Don't test 'const' and 'noreturn' because they are reserved anyway.
 #define abi_tag 1
-#define always_inline 1
 #ifndef __APPLE__
 // darwin headers use these, see PR 64883
-# define visibility 1
+# define always_inline 1
 # define deprecated 1
+# define visibility 1
 #endif
 #define packed 1
 #define pure 1
diff --git a/libstdc++-v3/testsuite/17_intro/headers/c++2014/all_attributes.cc 
b/libstdc++-v3/testsuite/17_intro/headers/c++2014/all_attributes.cc
index dbea4feec55..d6cc2c9f9db 100644
--- a/libstdc++-v3/testsuite/17_intro/headers/c++2014/all_attributes.cc
+++ b/libstdc++-v3/testsuite/17_intro/headers/c++2014/all_attributes.cc
@@ -21,9 +21,9 @@
 // Ensure the library only uses the __name__ form for attributes.
 // Don't test 'const' and 'noreturn' because they are reserved anyway.
 #define abi_tag 1
-#define always_inline 1
 #ifndef __APPLE__
 // darwin headers use these, see PR 64883
+# define always_inline 1
 # define deprecated 1
 # define visibility 1
 #endif
diff --git a/libstdc++-v3/testsuite/17_intro/headers/c++2017/all_attributes.cc 
b/libstdc++-v3/testsuite/17_intro/headers/c++2017/all_attributes.cc
index 0a92ae683a4..9f60190930d 100644
--- a/libstdc++-v3/testsuite/17_intro/headers/c++2017/all_attributes.cc
+++ b/libstdc++-v3/testsuite/17_intro/headers/c++2017/all_attributes.cc
@@ -21,9 +21,9 @@
 // Ensure the library only uses the __name__ form for attributes.
 // Don't test 'const' and 'noreturn' because they are reserved anyway.
 #define abi_tag 1
-#define always_inline 1
 #ifndef __APPLE__
 // darwin headers use this, see PR 64883
+# define always_inline 1
 # define visibility 1
 #endif
 #define packed 1
diff --git a/libstdc++-v3/testsuite/17_intro/headers/c++2020/all_attributes.cc 
b/libstdc++-v3/testsuite/17_intro/headers/c++2020/all_attributes.cc
index 766c7b68617..a5fca4c5311 100644
--- a/libstdc++-v3/testsuite/17_intro/headers/c++2020/all_attributes.cc
+++ b/libstdc++-v3/testsuite/17_intro/headers/c++2020/all_attributes.cc
@@ -21,9 +21,9 @@
 // Ensure the library only uses the __name__ form for attributes.
 // Don't test 'const' and 'noreturn' because they are reserved 

Re: Fortran patches

[Steve Kargl]

On Thu, Dec 06, 2018 at 08:02:43PM +0100, Thomas Koenig wrote:
> Hi Steve,
> 
> >>> PR fortran/88139
> >>> * dump-parse-tree.c (write_proc): Alternate return.
> >> I dissent with this patch. The introduced error is meaningless and, as
> >> mentioned by comment #3 in the PR, avoiding the ICE in dump-parse-tree
> >> is not directly the issue. The code should be rejected in parsing. In
> >> gcc-8.1 the invalid code is accepted (without an ICE) even without the
> >> -fc-prototypes flag: I haven't finished building the compiler with
> >> your changes yet to see whether that is still true afterwards, but at
> >> least the test case doesn't try this, so I strongly suspect the patch
> >> is incomplete to fix the PR.
> >   
> > Comment #3 does not contain a patch to fix the problem elsewhere.
> 
> I know :-)
> 
> > In F2003, 15.2.6 "Interoperability of procedures and procedure interfaces",
> > I cannot find a prohibition on an alternate return in a subroutine
> > interface with BIND(C).
> 
> I also does not allow this, and does not offer a valid interpretation
> of what it should mean.
> 
> If it has a meaning, it should be translatable into something prescribed
> by the standard with -fc-prototypes.
> 
> I have assigned the error to myself, so I will not forget to fix
> it before the gcc 9 release.

I think it comes down to F2018, 18.3.7, where one has

  A Fortran procedure interface is interoperable with a C functioni
  prototype if

   (1) ...
   (2) ...
   (3) ...
   (4) ...
   (5) any dummy argument without the VALUE attribute corresponds to
   a formal parameter of the prototype that is of a pointer type,
   and either (4 bullets which cannot be satisfied).

I suppose we should check what other compilers do on the 
testcase, but I only have access to gfortran.

BTW, write_proc() starts to write out the prototype before the
argument list is checked.  If the current gfc_error
is trigger, you get 

void foo (Error: Cannot convert %qs to interoperable type... 

I think you want to scan the formal argument list for errors
before writing out "void foo (".

-- 
Steve

Re: Fortran patches

[Fritz Reese]

On Wed, Dec 5, 2018 at 7:03 PM Steve Kargl
 wrote:
>
> On Wed, Dec 05, 2018 at 04:48:28PM -0500, Fritz Reese wrote:
[...]
> > RE:
> > > PR fortran/88228
> > > * expr.c (check_null, check_elemental): Work around -fdec and
> > > initialization with logical operators operating on integers.
> >
> > I plan to review this section of the patch later today -- though the
> > patch hides the segfault from the PR, I need more time to determine
> > whether it is correct and complete.
>
> By the time the gfc_expr is given to check_check and check_elemental,
> it has been reduced to a EXPR_CONSTANT, which neither routine expected.
> I simply return early in that case.

It appears the correct solution is simply the following patch:

diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index b2090218d48..775a5c52c65 100644
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -4004,7 +4004,7 @@ resolve_operator (gfc_expr *e)
  if (op2->ts.type != e->ts.type || op2->ts.kind != e->ts.kind)
gfc_convert_type (op2, >ts, 1);
  e = logical_to_bitwise (e);
- return resolve_function (e);
+ break;
}

   sprintf (msg, _("Operands of logical operator %%<%s%%> at %%L
are %s/%s"),
@@ -4020,7 +4020,7 @@ resolve_operator (gfc_expr *e)
  e->ts.type = BT_INTEGER;
  e->ts.kind = op1->ts.kind;
  e = logical_to_bitwise (e);
- return resolve_function (e);
+ break;
}

   if (op1->ts.type == BT_LOGICAL)

Returning immediately short-circuits various checks and
simplifications which are done in the remainder of resolve_operator,
including gfc_simplify_expr which handles the EXPR_CONSTANT case. The
comments on gfc_reduce_init_expr indicate that check_null and
check_elemental should never get EXPR_CONSTANT anyway if
gfc_resolve_expr is correct. Regression tests verify this patch is
correct. Please use this patch instead for PR 88228, or if you prefer
I can submit/commit the patch myself.

>
> > RE:
> > >PR fortran/88139
> > >* dump-parse-tree.c (write_proc): Alternate return.
> > I dissent with this patch. The introduced error is meaningless and, as
> > mentioned by comment #3 in the PR, avoiding the ICE in dump-parse-tree
> > is not directly the issue. The code should be rejected in parsing. In
> > gcc-8.1 the invalid code is accepted (without an ICE) even without the
> > -fc-prototypes flag: I haven't finished building the compiler with
> > your changes yet to see whether that is still true afterwards, but at
> > least the test case doesn't try this, so I strongly suspect the patch
> > is incomplete to fix the PR.
>
> Comment #3 does not contain a patch to fix the problem elsewhere.
>
> In F2003, 15.2.6 "Interoperability of procedures and procedure interfaces",
> I cannot find a prohibition on an alternate return in a subroutine
> interface with BIND(C).
>
> I'm disinclined to let a patch fester in bugzilla to only attain
> the same fate as my patch to PR68544.

According to F2008 §15.3.7.2(5):

> any dummy argument without the VALUE attribute [...] is interoperable with an 
> entity of the
> referenced type (ISO/IEC 9899:1999, 6.2.5, 7.17, and 7.18.1) of the formal 
> parameter

Regardless of whether or not we accept alternate returns in BIND(C)
procedures, the compiler must be at least consistent: if we accept
them (which gfortran currently does), then we should be able to dump
the C prototype (with -fc-prototypes), providing a formal parameter
interoperable with the type of the alternate return dummy argument; if
we reject them, then we should issue the error in parsing (before
handling by -fc-prototypes). In either case, the error message should
not be obscure or meaningless. Even so, the patch here is inconsistent
since we accept the code, but issue an error when attempting to dump
the C prototype.

However, gfortran does not implement alternate return dummy arguments
as actual arguments, but rather using an integer return code
(regardless of the number of alternate return parameters in the
interface). One interpretation of the consequences of this are that
BIND(C) should be rejected, since there is no interoperable formal
parameter which can be used to mirror the dummy argument (required by
15.3.7.2.5 above). An alternate interpretation is that we can continue
to accept BIND(C) with alternate return dummy arguments, but just
ignore the alternate return arguments. The former is perhaps more
"correct"; the latter is perhaps more useful albeit potentially
error-prone.

To patch support for the latter case, rather than issuing an error in
write_proc for procedures with alternate return arguments, we should
output the actual interoperable prototype: in this case we would
output 'int' as the return type (rather than void, as usual for
subroutines) and alternate return dummy arguments would be ignored
(not output). So the output for the example in the PR should really be
'int f()'. Something 

Re: Fortran patches

[Thomas Koenig]

Hi Steve,


PR fortran/88139
* dump-parse-tree.c (write_proc): Alternate return.

I dissent with this patch. The introduced error is meaningless and, as
mentioned by comment #3 in the PR, avoiding the ICE in dump-parse-tree
is not directly the issue. The code should be rejected in parsing. In
gcc-8.1 the invalid code is accepted (without an ICE) even without the
-fc-prototypes flag: I haven't finished building the compiler with
your changes yet to see whether that is still true afterwards, but at
least the test case doesn't try this, so I strongly suspect the patch
is incomplete to fix the PR.
  
Comment #3 does not contain a patch to fix the problem elsewhere.


I know :-)


In F2003, 15.2.6 "Interoperability of procedures and procedure interfaces",
I cannot find a prohibition on an alternate return in a subroutine
interface with BIND(C).


I also does not allow this, and does not offer a valid interpretation
of what it should mean.

If it has a meaning, it should be translatable into something prescribed
by the standard with -fc-prototypes.

I have assigned the error to myself, so I will not forget to fix
it before the gcc 9 release.

Regards

Thomas

Re: [PATCH v2 0/2] asm qualifiers (PR55681) and asm inline

[Segher Boessenkool]

On Thu, Dec 06, 2018 at 07:15:20PM +0100, Jakub Jelinek wrote:
> On Thu, Dec 06, 2018 at 12:10:56PM -0600, Segher Boessenkool wrote:
> > On Sun, Dec 02, 2018 at 04:38:16PM +, Segher Boessenkool wrote:
> > > v2, with the input from Joseph taken into account.
> > > 
> > > This is the same "asm inline" patch as before, but now preceded by a
> > > patch that makes all orderings of volatile/goto/inline valid, all other
> > > type qualifiers invalid, all repetitions of qualifiers invalid.
> > 
> > Committed now, with everyone's suggestions addressed.
> > 
> > Is this okay for backport to 8?  Maybe 7?  After a week or so, of course.
> > This will help the Linux people to use it sooner.
> 
> Not sure if in the backport we shouldn't keep accepting with warning like
> before const asm and not do the changes of accepting in any order except
> perhaps for the inline keyword in there?

Okay, I'll edit the const (etc.) back in.  The "any order" is easier to
keep this way I think (and doesn't change anything for code that was
already accepted).


Segher

Another patch for PR88282

[Vladimir Makarov]

  Here is another solution for

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88282

  less hackish than original one.

  The patch was bootstrapped and tested on x86/x86-64/ppc64/aarch64.

  Committed as rev. 266862.


Index: ChangeLog
===
--- ChangeLog	(revision 266861)
+++ ChangeLog	(working copy)
@@ -1,3 +1,11 @@
+2018-12-06  Vladimir Makarov  
+
+	PR target/88282
+	* ira.c (ira_init_register_move_cost): Use info from
+	hard_regno_mode_ok instead of contains_reg_of_mode.
+	* ira-costs.c (contains_reg_of_mode): Don't use cost from bigger
+	hard register class for some fixed hard registers.
+
 2018-12-06  Segher Boessenkool  
 
 	* doc/extend.texi (Using Assembly Language with C): Document asm inline.
Index: ira.c
===
--- ira.c	(revision 266861)
+++ ira.c	(working copy)
@@ -1573,11 +1573,17 @@ ira_init_register_move_cost (machine_mod
 {
   static unsigned short last_move_cost[N_REG_CLASSES][N_REG_CLASSES];
   bool all_match = true;
-  unsigned int cl1, cl2;
+  unsigned int i, cl1, cl2;
+  HARD_REG_SET ok_regs;
 
   ira_assert (ira_register_move_cost[mode] == NULL
 	  && ira_may_move_in_cost[mode] == NULL
 	  && ira_may_move_out_cost[mode] == NULL);
+  CLEAR_HARD_REG_SET (ok_regs);
+  for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
+if (targetm.hard_regno_mode_ok (i, mode))
+  SET_HARD_REG_BIT (ok_regs, i);
+
   /* Note that we might be asked about the move costs of modes that
  cannot be stored in any hard register, for example if an inline
  asm tries to create a register operand with an impossible mode.
@@ -1586,8 +1592,8 @@ ira_init_register_move_cost (machine_mod
 for (cl2 = 0; cl2 < N_REG_CLASSES; cl2++)
   {
 	int cost;
-	if (!contains_reg_of_mode[cl1][mode]
-	|| !contains_reg_of_mode[cl2][mode])
+	if (!hard_reg_set_intersect_p (ok_regs, reg_class_contents[cl1])
+	|| !hard_reg_set_intersect_p (ok_regs, reg_class_contents[cl2]))
 	  {
 	if ((ira_reg_class_max_nregs[cl1][mode]
 		 > ira_class_hard_regs_num[cl1])
Index: ira-costs.c
===
--- ira-costs.c	(revision 266784)
+++ ira-costs.c	(working copy)
@@ -1323,14 +1323,6 @@ record_operand_costs (rtx_insn *insn, en
 	  move_costs = ira_register_move_cost[mode];
 	  hard_reg_class = REGNO_REG_CLASS (other_regno);
 	  bigger_hard_reg_class = ira_pressure_class_translate[hard_reg_class];
-	  if (bigger_hard_reg_class == NO_REGS
-	  && (other_regno == STACK_POINTER_REGNUM
-#ifdef STATIC_CHAIN_REGNUM
-		  || other_regno == STATIC_CHAIN_REGNUM
-#endif
-		  || other_regno == FRAME_POINTER_REGNUM
-		  || other_regno == HARD_FRAME_POINTER_REGNUM))
-	bigger_hard_reg_class = GENERAL_REGS;
 	  /* Target code may return any cost for mode which does not
 	 fit the the hard reg class (e.g. DImode for AREG on
 	 i386).  Check this and use a bigger class to get the
@@ -1345,17 +1337,6 @@ record_operand_costs (rtx_insn *insn, en
 	  cost = (i == 0
 		  ? move_costs[hard_reg_class][rclass]
 		  : move_costs[rclass][hard_reg_class]);
-	  /* Target code might define wrong big costs for smaller
-		 reg classes or reg classes containing only fixed hard
-		 regs.  Try a bigger class.  */
-	  if (bigger_hard_reg_class != hard_reg_class)
-		{
-		  int cost2 = (i == 0
-			   ? move_costs[bigger_hard_reg_class][rclass]
-			   : move_costs[rclass][bigger_hard_reg_class]);
-		  if (cost2 < cost)
-		cost = cost2;
-		}
 	  
 	  op_costs[i]->cost[k] = cost * frequency;
 	  /* If we have assigned a class to this allocno in our

Re: [PATCH v2 0/2] asm qualifiers (PR55681) and asm inline

[Joseph Myers]

On Thu, 6 Dec 2018, Jakub Jelinek wrote:

> Not sure if in the backport we shouldn't keep accepting with warning like
> before const asm and not do the changes of accepting in any order except
> perhaps for the inline keyword in there?

Indeed, I think it's best to keep accepting const and restrict in asm for 
any backports, to avoid breaking any existing code with those constructs 
with a release branch change.

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

Re: [PATCH v2 0/2] asm qualifiers (PR55681) and asm inline

[Jakub Jelinek]

On Thu, Dec 06, 2018 at 12:10:56PM -0600, Segher Boessenkool wrote:
> On Sun, Dec 02, 2018 at 04:38:16PM +, Segher Boessenkool wrote:
> > v2, with the input from Joseph taken into account.
> > 
> > This is the same "asm inline" patch as before, but now preceded by a
> > patch that makes all orderings of volatile/goto/inline valid, all other
> > type qualifiers invalid, all repetitions of qualifiers invalid.
> 
> Committed now, with everyone's suggestions addressed.
> 
> Is this okay for backport to 8?  Maybe 7?  After a week or so, of course.
> This will help the Linux people to use it sooner.

Not sure if in the backport we shouldn't keep accepting with warning like
before const asm and not do the changes of accepting in any order except
perhaps for the inline keyword in there?

Jakub

Re: [PATCH v2 0/2] asm qualifiers (PR55681) and asm inline

[Segher Boessenkool]

Hi all,

On Sun, Dec 02, 2018 at 04:38:16PM +, Segher Boessenkool wrote:
> v2, with the input from Joseph taken into account.
> 
> This is the same "asm inline" patch as before, but now preceded by a
> patch that makes all orderings of volatile/goto/inline valid, all other
> type qualifiers invalid, all repetitions of qualifiers invalid.

Committed now, with everyone's suggestions addressed.

Is this okay for backport to 8?  Maybe 7?  After a week or so, of course.
This will help the Linux people to use it sooner.


Segher

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v5]

[Ian Lance Taylor via gcc-patches]

On Thu, Dec 6, 2018 at 3:12 AM Nick Clifton  wrote:
>
>   Is the patch OK with you ?

Yes, thanks.

Ian

Remove bogus test line from vect-over-widen-23.c

[Richard Sandiford]

I'd cut-&-paste vect-over-widen-23.c in an attempt to get the target
reuirements right, but by doing so carried over an unwanted test for
shifts.

Tested on arm-none-eabi and committed as obvious.

Richard


2018-12-06  Richard Sandiford  

gcc/testsuite/
* gcc.dg/vect/vect-over-widen-23.c: Remove unwanted line.

Index: gcc/testsuite/gcc.dg/vect/vect-over-widen-23.c
===
--- gcc/testsuite/gcc.dg/vect/vect-over-widen-23.c  2018-12-05 
15:52:39.706005365 +
+++ gcc/testsuite/gcc.dg/vect/vect-over-widen-23.c  2018-12-06 
18:00:59.833858720 +
@@ -25,7 +25,6 @@ foo ()
 }
 }
 
-/* { dg-final { scan-tree-dump-times "vect_recog_widen_shift_pattern: 
detected" 2 "vect" { target vect_widen_shift } } } */
 /* { dg-final { scan-tree-dump {vect_recog_over_widening_pattern: 
detected:[^\n]* \+} "vect" } } */
 /* { dg-final { scan-tree-dump {VIEW_CONVERT_EXPR

Re: [RFC PATCH] Coalesce host to device transfers in libgomp

[Jakub Jelinek]

On Thu, Dec 06, 2018 at 06:54:20PM +0100, Thomas Schwinge wrote:
> On Thu, 6 Dec 2018 18:18:56 +0100, Jakub Jelinek  wrote:
> > On Thu, Dec 06, 2018 at 06:01:48PM +0100, Thomas Schwinge wrote:
> > > While reviewing Chung-Lin's
> > >  "[PATCH 4/6,
> > > OpenACC, libgomp] Async re-work, libgomp/target.c changes", I noticed the
> > > following unrelated hunk.  Is that intentional or just an oversight that
> > > it hasn't been included in your "gomp_coalesce_buf" changes (quoted below
> > > for reference)?
> > 
> > I believe it is intentional, the coalescing code coalesces only stuff
> > allocated by the current gomp_map_vars call, for the link_key case we know
> > that is not the case, it is a copy to a file scope data variable in the PTX
> > code.
> 
> Hmm, I thought this would just copy an address (as opposed to data) from
> the host to the device, so that would be fine for coalescing.  But I'm
> not familiar with that code, so it's certainly possible that I'm not
> understanding this correctly.

The actual data transfer can be coalesced, just the address is copied into
the offloaded file scope var and so that exact transfer can't be coalesced.

> > Perhaps we could do the change but pass NULL instead
> > of cbufp as the last argument?
> 
> Like this?
> 
> commit 241027a03b70c788ef94ccf258b799332fb1b20e
> Author: Thomas Schwinge 
> Date:   Thu Dec 6 18:53:16 2018 +0100
> 
> Coalesce host to device transfers in libgomp: not for link pointer
> 
> 2018-12-06  Thomas Schwinge  
> Jakub Jelinek  
> 
> libgomp/
> * target.c (gomp_map_vars): Call "gomp_copy_host2dev" instead of
> "devicep->host2dev_func".

Ok for trunk, thanks.  Perhaps no need for the "s in the ChangeLog.

> ---
>  libgomp/target.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git libgomp/target.c libgomp/target.c
> index 8ebc2a370a16..60f4c96f3908 100644
> --- libgomp/target.c
> +++ libgomp/target.c
> @@ -957,9 +957,11 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t 
> mapnum,
>   /* Set link pointer on target to the device address of the
>  mapped object.  */
>   void *tgt_addr = (void *) (tgt->tgt_start + k->tgt_offset);
> - devicep->host2dev_func (devicep->target_id,
> - (void *) n->tgt_offset,
> - _addr, sizeof (void *));
> + /* We intentionally do not use coalescing here, as it's not
> +data allocated by the current call to this function.  */
> + gomp_copy_host2dev (devicep, (void *) n->tgt_offset,
> + _addr, sizeof (void *), NULL);
> +
> }
>   array++;
> }

Jakub

Re: [RFC PATCH] Coalesce host to device transfers in libgomp

[Thomas Schwinge]

Hi Jakub!

On Thu, 6 Dec 2018 18:18:56 +0100, Jakub Jelinek  wrote:
> On Thu, Dec 06, 2018 at 06:01:48PM +0100, Thomas Schwinge wrote:
> > While reviewing Chung-Lin's
> >  "[PATCH 4/6,
> > OpenACC, libgomp] Async re-work, libgomp/target.c changes", I noticed the
> > following unrelated hunk.  Is that intentional or just an oversight that
> > it hasn't been included in your "gomp_coalesce_buf" changes (quoted below
> > for reference)?
> 
> I believe it is intentional, the coalescing code coalesces only stuff
> allocated by the current gomp_map_vars call, for the link_key case we know
> that is not the case, it is a copy to a file scope data variable in the PTX
> code.

Hmm, I thought this would just copy an address (as opposed to data) from
the host to the device, so that would be fine for coalescing.  But I'm
not familiar with that code, so it's certainly possible that I'm not
understanding this correctly.

> Perhaps we could do the change but pass NULL instead
> of cbufp as the last argument?

Like this?

commit 241027a03b70c788ef94ccf258b799332fb1b20e
Author: Thomas Schwinge 
Date:   Thu Dec 6 18:53:16 2018 +0100

Coalesce host to device transfers in libgomp: not for link pointer

2018-12-06  Thomas Schwinge  
Jakub Jelinek  

libgomp/
* target.c (gomp_map_vars): Call "gomp_copy_host2dev" instead of
"devicep->host2dev_func".
---
 libgomp/target.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git libgomp/target.c libgomp/target.c
index 8ebc2a370a16..60f4c96f3908 100644
--- libgomp/target.c
+++ libgomp/target.c
@@ -957,9 +957,11 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t 
mapnum,
/* Set link pointer on target to the device address of the
   mapped object.  */
void *tgt_addr = (void *) (tgt->tgt_start + k->tgt_offset);
-   devicep->host2dev_func (devicep->target_id,
-   (void *) n->tgt_offset,
-   _addr, sizeof (void *));
+   /* We intentionally do not use coalescing here, as it's not
+  data allocated by the current call to this function.  */
+   gomp_copy_host2dev (devicep, (void *) n->tgt_offset,
+   _addr, sizeof (void *), NULL);
+
  }
array++;
  }


Grüße
 Thomas

Re: [PATCH 4/6, OpenACC, libgomp] Async re-work, libgomp/target.c changes

[Jakub Jelinek]

On Thu, Dec 06, 2018 at 06:21:16PM +0100, Thomas Schwinge wrote:
> On Tue, 25 Sep 2018 21:11:24 +0800, Chung-Lin Tang  
> wrote:
> > Hi Jakub,
> > This part has changes to 'struct goacc_asyncqueue*' arguments to various
> > memory copying/mapping functions. To lessen the amount of code changes new 
> > 'gomp_map/unmap_vars_async'
> > functions names are used (with the non-async original names defined with 
> > the asyncqueue==NULL).
> 
> Is that the way you'd like this to be done, or should instead that
> "struct goacc_asyncqueue *aq" parameter be added/passed through all the
> existing functions?  (The latter would be my preference, actually.)

I'd prefer not to increase the amount of arguments where possible, because
many of the functions already have more arguments than can be passed in
registers.  Could it be e.g. added into gomp_coalesce_buf which is already
passed around?

Another option would be to use always_inline as C template if the OpenMP and
OpenACC needs diverge too much, then have simply small wrappers that just
call the always_inline function, in one case with the argument NULL or other
constant, in another one with whatever it has been called with.

Jakub

Re: [C++ Patch] Three additional bitfield diagnostic tweaks (a regression fix included)

[Paolo Carlini]

Hi,

On 06/12/18 16:11, Jason Merrill wrote:
2- Unfortunately I have to fix another buglet I recently introduced, 
completely similar to c++/88222 fixed by Marek. Well, at least we will 
not print anymore an empty '' when the unqualified_id is null because 
the field is unnamed.



-    error_at (declarator->id_loc,
-  "%qE is neither function nor member function; "
-  "cannot be declared friend", unqualified_id);
+    if (unqualified_id && declarator)
+  error_at (declarator->id_loc,
+    "%qE is neither function nor member function; "
+    "cannot be declared friend", unqualified_id);
+    else
+  error ("unnamed field is neither function nor member "
+ "function; cannot be declared friend");


I wonder if we want to use the 'name' variable here.


Well, the name variable doesn't seem that useful here because for the 
new testcase it has that famous catch all value "type name" .


I have been thinking that here and in other places we could imagine 
keeping only the declarator check and dropping the "name" check. 
Probably it would work. But in *many* existing places we actually check 
*only* the name thus I'm nervous about attempting that now...




3- In the non-static case too, when from grokdeclarator we are 
calling FIELD_DECL and passing the location as first argument, I 
think we want to likewise pass declarator->id_loc when available.



-    decl = build_decl (input_location,
+    decl = build_decl (declarator
+   ? declarator->id_loc
+   : input_location,


I think we want to put this in a local variable, to share with the 
static case and probably other places in grokdeclarator.


In the below I'm sharing it only with the static case, straightforward. 
Moving it one level up doesn't seem that useful because we only have 
rather safe IMHO unconditional uses either of input_location or of 
declarator->id_loc at the moment... Again, I'm pretty sure there is room 
for further clean-ups in this area, but, for 9, I'd rather take care of 
a bunch of additional small issues which I already have in my TODO list, 
in grokbitfield, for example, as already mentioned. By the way, if isn't 
already clear, I have been changing location bits only when I already 
have a set of testcases, constructed from our testsuite via (lenghty ;) 
instrumented runs.


New version of the patch attached.

Paolo.

Index: cp/class.c
===
--- cp/class.c  (revision 266840)
+++ cp/class.c  (working copy)
@@ -3218,7 +3218,8 @@ check_bitfield_decl (tree field)
   /* Detect invalid bit-field type.  */
   if (!INTEGRAL_OR_ENUMERATION_TYPE_P (type))
 {
-  error ("bit-field %q+#D with non-integral type", field);
+  error_at (DECL_SOURCE_LOCATION (field),
+   "bit-field %q#D with non-integral type %qT", field, type);
   w = error_mark_node;
 }
   else
Index: cp/decl.c
===
--- cp/decl.c   (revision 266840)
+++ cp/decl.c   (working copy)
@@ -12446,9 +12446,13 @@ grokdeclarator (const cp_declarator *declarator,
  {
if (friendp)
  {
-   error_at (declarator->id_loc,
- "%qE is neither function nor member function; "
- "cannot be declared friend", unqualified_id);
+   if (unqualified_id && declarator)
+ error_at (declarator->id_loc,
+   "%qE is neither function nor member function; "
+   "cannot be declared friend", unqualified_id);
+   else
+ error ("unnamed field is neither function nor member "
+"function; cannot be declared friend");
return error_mark_node;
  }
decl = NULL_TREE;
@@ -12483,14 +12487,13 @@ grokdeclarator (const cp_declarator *declarator,
 
if (decl == NULL_TREE)
  {
+   location_t loc = declarator ? declarator->id_loc : input_location;
if (staticp)
  {
/* C++ allows static class members.  All other work
   for this is done by grokfield.  */
-   decl = build_lang_decl_loc (declarator
-   ? declarator->id_loc
-   : input_location,
-   VAR_DECL, unqualified_id, type);
+   decl = build_lang_decl_loc (loc, VAR_DECL,
+   unqualified_id, type);
set_linkage_for_static_data_member (decl);
if (concept_p)
  error_at (declspecs->locations[ds_concept],
@@ -12536,8 +12539,7 @@ grokdeclarator (const cp_declarator *declarator,
  unqualified_id);

Re: [PATCH 4/6, OpenACC, libgomp] Async re-work, libgomp/target.c changes

[Thomas Schwinge]

Hi Jakub!

On Tue, 25 Sep 2018 21:11:24 +0800, Chung-Lin Tang  
wrote:
> Hi Jakub,
> This part has changes to 'struct goacc_asyncqueue*' arguments to various
> memory copying/mapping functions. To lessen the amount of code changes new 
> 'gomp_map/unmap_vars_async'
> functions names are used (with the non-async original names defined with the 
> asyncqueue==NULL).

Is that the way you'd like this to be done, or should instead that
"struct goacc_asyncqueue *aq" parameter be added/passed through all the
existing functions?  (The latter would be my preference, actually.)

That is, as Chung-Lin proposed:

> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -177,6 +177,22 @@ gomp_device_copy (struct gomp_device_descr *devicep,
>  }
>  }
>  
> +static inline void
> +goacc_device_copy_async (struct gomp_device_descr *devicep,
> +  bool (*copy_func) (int, void *, const void *, size_t,
> + struct goacc_asyncqueue *),
> +  const char *dst, void *dstaddr,
> +  const char *src, const void *srcaddr,
> +  size_t size, struct goacc_asyncqueue *aq)
> +{
> +  if (!copy_func (devicep->target_id, dstaddr, srcaddr, size, aq))
> +{
> +  gomp_mutex_unlock (>lock);
> +  gomp_fatal ("Copying of %s object [%p..%p) to %s object [%p..%p) 
> failed",
> +   src, srcaddr, srcaddr + size, dst, dstaddr, dstaddr + size);
> +}
> +}

..., or should we instead add "struct goacc_asyncqueue *aq" to the
existing "gomp_device_copy", and then, recursively, also add it to the
existing plugin functions "host2dev" and "dev2host", instead of adding
new functions "openacc.async.host2dev" and "openacc.async.dev2host" (see
"GOMP_OFFLOAD_host2dev" vs. "GOMP_OFFLOAD_openacc_async_host2dev", and
"GOMP_OFFLOAD_dev2host" vs. "GOMP_OFFLOAD_openacc_async_dev2host" as
proposed in 
"[PATCH 6/6, OpenACC, libgomp] Async re-work, nvptx changes")?

Similarly for "gomp_map_vars"/"gomp_map_vars_async",
"gomp_unmap_vars"/"gomp_unmap_vars_async", see below.

I'd rather have one single interface (optionally called with a "NULL"
"struct goacc_asyncqueue *aq"), instead of adding more/similar async
interfaces.  Aside from avoiding adding to the cognitive load, the
rationaly also being that in the long term, for performance reasons,
we'll probably want to make more stuff asynchronous that currently is
synchronous, thus eventually obsoleting the synchronous interfaces.

For reference:

> @@ -263,8 +279,9 @@ gomp_to_device_kind_p (int kind)
>  }
>  }
>  
> -static void
> +attribute_hidden void
>  gomp_copy_host2dev (struct gomp_device_descr *devicep,
> + struct goacc_asyncqueue *aq,
>   void *d, const void *h, size_t sz,
>   struct gomp_coalesce_buf *cbuf)
>  {
> @@ -293,14 +310,23 @@ gomp_copy_host2dev (struct gomp_device_descr *devicep,
>   }
>   }
>  }
> -  gomp_device_copy (devicep, devicep->host2dev_func, "dev", d, "host", h, 
> sz);
> +  if (aq)
> +goacc_device_copy_async (devicep, devicep->openacc.async.host2dev_func,
> +  "dev", d, "host", h, sz, aq);
> +  else
> +gomp_device_copy (devicep, devicep->host2dev_func, "dev", d, "host", h, 
> sz);
>  }
>  
> -static void
> +attribute_hidden void
>  gomp_copy_dev2host (struct gomp_device_descr *devicep,
> + struct goacc_asyncqueue *aq,
>   void *h, const void *d, size_t sz)
>  {
> -  gomp_device_copy (devicep, devicep->dev2host_func, "host", h, "dev", d, 
> sz);
> +  if (aq)
> +goacc_device_copy_async (devicep, devicep->openacc.async.dev2host_func,
> +  "host", h, "dev", d, sz, aq);
> +  else
> +gomp_device_copy (devicep, devicep->dev2host_func, "host", h, "dev", d, 
> sz);
>  }
>  
>  static void
> @@ -318,7 +344,8 @@ gomp_free_device_memory (struct gomp_device_descr 
> *devicep, void *devptr)
> Helper function of gomp_map_vars.  */
>  
>  static inline void
> -gomp_map_vars_existing (struct gomp_device_descr *devicep, splay_tree_key 
> oldn,
> +gomp_map_vars_existing (struct gomp_device_descr *devicep,
> + struct goacc_asyncqueue *aq, splay_tree_key oldn,
>   splay_tree_key newn, struct target_var_desc *tgt_var,
>   unsigned char kind, struct gomp_coalesce_buf *cbuf)
>  {
> @@ -340,7 +367,7 @@ gomp_map_vars_existing (struct gomp_device_descr 
> *devicep, splay_tree_key oldn,
>  }
>  
>if (GOMP_MAP_ALWAYS_TO_P (kind))
> -gomp_copy_host2dev (devicep,
> +gomp_copy_host2dev (devicep, aq,
>   (void *) (oldn->tgt->tgt_start + oldn->tgt_offset
> + newn->host_start - oldn->host_start),
>   (void *) newn->host_start,
> @@ -358,8 +385,8 @@ get_kind (bool short_mapkind, void *kinds, int idx)
>  }
>  
>  static 

Re: [RFC PATCH] Coalesce host to device transfers in libgomp

[Jakub Jelinek]

On Thu, Dec 06, 2018 at 06:01:48PM +0100, Thomas Schwinge wrote:
> While reviewing Chung-Lin's
>  "[PATCH 4/6,
> OpenACC, libgomp] Async re-work, libgomp/target.c changes", I noticed the
> following unrelated hunk.  Is that intentional or just an oversight that
> it hasn't been included in your "gomp_coalesce_buf" changes (quoted below
> for reference)?

I believe it is intentional, the coalescing code coalesces only stuff
allocated by the current gomp_map_vars call, for the link_key case we know
that is not the case, it is a copy to a file scope data variable in the PTX
code.  Perhaps we could do the change but pass NULL instead
of cbufp as the last argument?

> commit 2abec5454063076ebd0fddf6ed25a3459c4f5ac3
> Author: Thomas Schwinge 
> Date:   Thu Dec 6 17:52:34 2018 +0100
> 
> Coalesce host to device transfers in libgomp: link pointer
> 
> libgomp/
> * target.c (gomp_map_vars): Call "gomp_copy_host2dev" instead of
> "devicep->host2dev_func".
> ---
>  libgomp/target.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git libgomp/target.c libgomp/target.c
> index 8ebc2a370a16..9cb2ec8d026f 100644
> --- libgomp/target.c
> +++ libgomp/target.c
> @@ -957,9 +957,9 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t 
> mapnum,
>   /* Set link pointer on target to the device address of the
>  mapped object.  */
>   void *tgt_addr = (void *) (tgt->tgt_start + k->tgt_offset);
> - devicep->host2dev_func (devicep->target_id,
> - (void *) n->tgt_offset,
> - _addr, sizeof (void *));
> + gomp_copy_host2dev (devicep, (void *) n->tgt_offset,
> + _addr, sizeof (void *), cbufp);
> +
> }
>   array++;
> }
> 

Jakub

Re: [RFC PATCH] Coalesce host to device transfers in libgomp

[Thomas Schwinge]

Hi Jakub!

While reviewing Chung-Lin's
 "[PATCH 4/6,
OpenACC, libgomp] Async re-work, libgomp/target.c changes", I noticed the
following unrelated hunk.  Is that intentional or just an oversight that
it hasn't been included in your "gomp_coalesce_buf" changes (quoted below
for reference)?

commit 2abec5454063076ebd0fddf6ed25a3459c4f5ac3
Author: Thomas Schwinge 
Date:   Thu Dec 6 17:52:34 2018 +0100

Coalesce host to device transfers in libgomp: link pointer

libgomp/
* target.c (gomp_map_vars): Call "gomp_copy_host2dev" instead of
"devicep->host2dev_func".
---
 libgomp/target.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git libgomp/target.c libgomp/target.c
index 8ebc2a370a16..9cb2ec8d026f 100644
--- libgomp/target.c
+++ libgomp/target.c
@@ -957,9 +957,9 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t 
mapnum,
/* Set link pointer on target to the device address of the
   mapped object.  */
void *tgt_addr = (void *) (tgt->tgt_start + k->tgt_offset);
-   devicep->host2dev_func (devicep->target_id,
-   (void *) n->tgt_offset,
-   _addr, sizeof (void *));
+   gomp_copy_host2dev (devicep, (void *) n->tgt_offset,
+   _addr, sizeof (void *), cbufp);
+
  }
array++;
  }

If approving this patch, please respond with "Reviewed-by: NAME "
so that your effort will be recorded in the commit log, see
.


Grüße
 Thomas


On Wed, 25 Oct 2017 13:38:50 +0200, Jakub Jelinek  wrote:
> Here is an updated patch with some renaming, extra macros, one extra inline
> function and comments.

> 2017-10-25  Jakub Jelinek  
> 
>   * target.c (struct gomp_coalesce_buf): New type.
>   (MAX_COALESCE_BUF_SIZE, MAX_COALESCE_BUF_GAP): Define.
>   (gomp_coalesce_buf_add, gomp_to_device_kind_p): New functions.
>   (gomp_copy_host2dev): Add CBUF argument, if copying into
>   the cached ranges, memcpy into buffer instead of copying
>   into device.
>   (gomp_map_vars_existing, gomp_map_pointer, gomp_map_fields_existing):
>   Add CBUF argument, pass it through to other calls.
>   (gomp_map_vars): Aggregate copies from host to device if small enough
>   and with small enough gaps in between into memcpy into a buffer and
>   fewer host to device copies from the buffer.
>   (gomp_update): Adjust gomp_copy_host2dev caller.
> 
> --- libgomp/target.c.jj   2017-10-24 12:07:03.763759657 +0200
> +++ libgomp/target.c  2017-10-25 13:17:31.608975390 +0200
> @@ -177,10 +177,122 @@ gomp_device_copy (struct gomp_device_des
>  }
>  }
>  
> +/* Infrastructure for coalescing adjacent or nearly adjacent (in device 
> addresses)
> +   host to device memory transfers.  */
> +
> +struct gomp_coalesce_buf
> +{
> +  /* Buffer into which gomp_copy_host2dev will memcpy data and from which
> + it will be copied to the device.  */
> +  void *buf;
> +  struct target_mem_desc *tgt;
> +  /* Array with offsets, chunks[2 * i] is the starting offset and
> + chunks[2 * i + 1] ending offset relative to tgt->tgt_start device 
> address
> + of chunks which are to be copied to buf and later copied to device.  */
> +  size_t *chunks;
> +  /* Number of chunks in chunks array, or -1 if coalesce buffering should not
> + be performed.  */
> +  long chunk_cnt;
> +  /* During construction of chunks array, how many memory regions are within
> + the last chunk.  If there is just one memory region for a chunk, we copy
> + it directly to device rather than going through buf.  */
> +  long use_cnt;
> +};
> +
> +/* Maximum size of memory region considered for coalescing.  Larger copies
> +   are performed directly.  */
> +#define MAX_COALESCE_BUF_SIZE(32 * 1024)
> +
> +/* Maximum size of a gap in between regions to consider them being copied
> +   within the same chunk.  All the device offsets considered are within
> +   newly allocated device memory, so it isn't fatal if we copy some padding
> +   in between from host to device.  The gaps come either from alignment
> +   padding or from memory regions which are not supposed to be copied from
> +   host to device (e.g. map(alloc:), map(from:) etc.).  */
> +#define MAX_COALESCE_BUF_GAP (4 * 1024)
> +
> +/* Add region with device tgt_start relative offset and length to CBUF.  */
> +
> +static inline void
> +gomp_coalesce_buf_add (struct gomp_coalesce_buf *cbuf, size_t start, size_t 
> len)
> +{
> +  if (len > MAX_COALESCE_BUF_SIZE || len == 0)
> +return;
> +  if (cbuf->chunk_cnt)
> +{
> +  if (cbuf->chunk_cnt < 0)
> + return;
> +  if (start < cbuf->chunks[2 * cbuf->chunk_cnt - 1])
> + {
> +   cbuf->chunk_cnt = -1;

C++ PATCH for c++/88373, wrong parse error with ~

[Marek Polacek]

This patch fixes a bogus parse error with ~ in a template-argument-list.  We 
have

  S>

and cp_parser_template_argument just tries to parse each argument as a type,
id-expression, etc to see what sticks.  When it sees ~value, it tries to parse
it using cp_parser_class_name (because of the ~), which ends up calling
cp_parser_lookup_name, looking for "value", but finds nothing.  Since it's an
unqualified-id followed by <, we treat "~value" as a template name
function.  It isn't followed by "(args)", so we simulate parse error.  As a
consequence of this error, the parsing of the outermost template-id S fails.

The problem is that when we're looking up the name in cp_parser_class_name,
tag_type is typename_type, which means bindings that do not refer to types
are ignored, so the variable template "value" isn't found and we're toast.

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

2018-12-06  Marek Polacek  

PR c++/88373 - wrong parse error with ~.
* parser.c (cp_parser_template_name): Check tag_type for
none_type.

* g++.dg/cpp2a/fn-template19.C: New test.

diff --git gcc/cp/parser.c gcc/cp/parser.c
index ac19cb4b9bb..2f55855ce9f 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -16579,7 +16579,8 @@ cp_parser_template_name (cp_parser* parser,
   if (!found
  && (cxx_dialect > cxx17)
  && !scoped_p
- && cp_lexer_next_token_is (parser->lexer, CPP_LESS))
+ && cp_lexer_next_token_is (parser->lexer, CPP_LESS)
+ && tag_type == none_type)
{
  /* [temp.names] says "A name is also considered to refer to a template
 if it is an unqualified-id followed by a < and name lookup finds
diff --git gcc/testsuite/g++.dg/cpp2a/fn-template19.C 
gcc/testsuite/g++.dg/cpp2a/fn-template19.C
new file mode 100644
index 000..1d6b43bb7ce
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp2a/fn-template19.C
@@ -0,0 +1,11 @@
+// PR c++/88373
+// { dg-do compile }
+// { dg-options "-std=c++2a" }
+
+template 
+constexpr T value = T {};
+
+template 
+struct S {};
+
+using U = S >;

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v5]

[Jason Merrill]

On 12/4/18 11:56 AM, Nick Clifton wrote:

OK, revised (v5) patch attached.  Is this version acceptable to all ?


Looks good to me.  Independently, do you see a reason not to disable the 
old demangler entirely?


Jason

Re: [PATCH 10/10] Port testsuite to GCN

[Andrew Stubbs]

On 26/11/2018 21:13, Mike Stump wrote:

On Nov 26, 2018, at 12:04 PM, Mike Stump  wrote:


I'll Ok the signal one, if you prefer it over a dummy signal routine.  Though, 
would be nice for you to add signal if possible/reasonable.


Oh, and my long term thinking on signal is that logically, it's fine to have:

#if __has_include("signal.h")
   signal(...);
#endif


This doesn't work here because we have signal.h (and indeed "signal").

The problem is that the function doesn't work (actually, it doesn't 
link, because "kill" is missing).


I've solved the problem by adding a machine-specific stub function in 
Newlib, so we can drop this portion of the patch from the testsuite now.


Andrew

[committed] PR testsuite/86540, twiddle for aarch64

[Jeff Law]

As outlined in the PR, the aarch64 has a non-default value for
CASE_VALUES_THRESHOLD which changes decisions in switch lowering.  Those
changes in switch lowering can expose additional jump threads later in
the pipeline which cause heartburn for a couple tests.

I looked at all the other ports with a non-default value of
CASE_VALUES_THRESHOLD and only aarch64 is high enough to trigger these
changes in behavior on the two relevant tests.  So I'm just skipping the
tests that run after switch lowering on aarch64.

Verified with a cross that these tests now pass.

Committing to the trunk,

Jeff
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 1adb751cd34..0272bbe0605 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2018-12-06  Jeff Law  
+
+   PR testsuite/86540
+   * gcc.dg/tree-ssa/ssa-dom-thread-7.c: Skip the post switch conversion
+   tests on aarch64.
+   * gcc.dg/tree-ssa/pr77445-2.c: Similarly.
+ 
 2018-12-06  David Malcolm  
 
PR c++/85110
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr77445-2.c 
b/gcc/testsuite/gcc.dg/tree-ssa/pr77445-2.c
index eecfc4b195a..c5d567dabdc 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr77445-2.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr77445-2.c
@@ -118,10 +118,14 @@ enum STATES FMS( u8 **in , u32 *transitions) {
 
 /* The profile is not updated perfectly because it is inconsitent from
profile estimation stage. But the number of inconsistencies should not
-   increase much.  */
+   increase much. 
+
+   aarch64 has the highest CASE_VALUES_THRESHOLD in GCC.  It's high enough
+   to change decisions in switch expansion which in turn can expose new
+   jump threading opportunities.  Skip the later tests on aarch64.  */
 /* { dg-final { scan-tree-dump "Jumps threaded: 1\[1-9\]" "thread1" } } */
 /* { dg-final { scan-tree-dump-times "Invalid sum" 3 "thread1" } } */
 /* { dg-final { scan-tree-dump-not "not considered" "thread1" } } */
 /* { dg-final { scan-tree-dump-not "not considered" "thread2" } } */
-/* { dg-final { scan-tree-dump-not "not considered" "thread3" } } */
-/* { dg-final { scan-tree-dump-not "not considered" "thread4" } } */
+/* { dg-final { scan-tree-dump-not "not considered" "thread3" { target { ! 
aarch64*-*-* } } } } */
+/* { dg-final { scan-tree-dump-not "not considered" "thread4" { target { ! 
aarch64*-*-* } } } } */ 
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-thread-7.c 
b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-thread-7.c
index e395de26ec0..f833aa4351d 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-thread-7.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-thread-7.c
@@ -3,8 +3,11 @@
 /* { dg-final { scan-tree-dump "Jumps threaded: 16"  "thread1" } } */
 /* { dg-final { scan-tree-dump "Jumps threaded: 9" "thread2" } } */
 /* { dg-final { scan-tree-dump "Jumps threaded: 1"  "dom2" } } */
-/* { dg-final { scan-tree-dump-not "Jumps threaded"  "dom3" } } */
-/* { dg-final { scan-tree-dump-not "Jumps threaded"  "vrp2" } } */
+/* aarch64 has the highest CASE_VALUES_THRESHOLD in GCC.  It's high enough
+   to change decisions in switch expansion which in turn can expose new
+   jump threading opportunities.  Skip the later tests on aarch64.  */
+/* { dg-final { scan-tree-dump-not "Jumps threaded"  "dom3" { target { ! 
aarch64*-*-* } } } } */
+/* { dg-final { scan-tree-dump-not "Jumps threaded"  "vrp2" { target { ! 
aarch64*-*-* } } } } */
 
 /* Most architectures get 3 threadable paths here, whereas aarch64 and
possibly others get 5.  We really should rewrite threading tests to

Re: [PATCH 10/10] Port testsuite to GCN

[Andrew Stubbs]

I finally got back to investigating this 

On 21/11/2018 01:00, Jeff Law wrote:

--- a/gcc/testsuite/gcc.dg/gimplefe-28.c
+++ b/gcc/testsuite/gcc.dg/gimplefe-28.c
@@ -1,5 +1,5 @@
  /* { dg-do compile { target sqrt_insn } } */
-/* { dg-options "-fgimple -O2" } */
+/* { dg-options "-fgimple -O2 -ffast-math" } */

So why does the GCN need fast-math here?  I'm not aware of any other
target  that needs that kind of handling to make this test work.


It needs it because the sqrt instruction is only enabled when 
flag_unsafe_math_optimizations is set. This seems appropriate given the 
approximate nature of the machine instruction.


This test uses gimple directly and so bypasses the usual optab checks 
that would normally select a library function instead, which results in 
an ICE.


It seems like a safe change to make, since most targets will have more 
patterns enabled, not fewer, in this mode.


The test will continue to fail on any target that does not have a sqrt 
instruction at all.


Andrew

Re: [C++ Patch] Three additional bitfield diagnostic tweaks (a regression fix included)

[Jason Merrill]

On 12/6/18 5:49 AM, Paolo Carlini wrote:

Hi,

I'm bundling together 3 more. In attachment order:

1- Since we decided to explicitly print the wrong type, I suppose we 
want to consistently do that in templates too.


OK.

2- Unfortunately I have to fix another buglet I recently introduced, 
completely similar to c++/88222 fixed by Marek. Well, at least we will 
not print anymore an empty '' when the unqualified_id is null because 
the field is unnamed.



-   error_at (declarator->id_loc,
- "%qE is neither function nor member function; "
- "cannot be declared friend", unqualified_id);
+   if (unqualified_id && declarator)
+ error_at (declarator->id_loc,
+   "%qE is neither function nor member function; "
+   "cannot be declared friend", unqualified_id);
+   else
+ error ("unnamed field is neither function nor member "
+"function; cannot be declared friend");


I wonder if we want to use the 'name' variable here.

3- In the non-static case too, when from grokdeclarator we are calling 
FIELD_DECL and passing the location as first argument, I think we want 
to likewise pass declarator->id_loc when available.



-   decl = build_decl (input_location,
+   decl = build_decl (declarator
+  ? declarator->id_loc
+  : input_location,


I think we want to put this in a local variable, to share with the 
static case and probably other places in grokdeclarator.


Jason

Re: [PATCH] v3: C++: improvements to diagnostics using %P (more PR c++/85110)

[Jason Merrill]

On 12/6/18 10:38 AM, David Malcolm wrote:

On Wed, 2018-12-05 at 19:49 -0500, Jason Merrill wrote:

On 12/3/18 5:54 PM, David Malcolm wrote:


[...]

Thanks for the review.  Here's a v3 of the patch; comments inline
below.


diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index ee099cc..cfc5641 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -6681,16 +6681,24 @@ conversion_null_warnings (tree totype, tree
expr, tree fn, int argnum)
 if (null_node_p (expr) && TREE_CODE (totype) != BOOLEAN_TYPE
 && ARITHMETIC_TYPE_P (totype))
   {
-  location_t loc =
-   expansion_point_location_if_in_system_header
(input_location);
-
 if (fn)
-   warning_at (loc, OPT_Wconversion_null,
-   "passing NULL to non-pointer argument %P of
%qD",
-   argnum, fn);
+   {
+ location_t loc = EXPR_LOC_OR_LOC (expr, input_location);
+ loc = expansion_point_location_if_in_system_header
(loc);
+ auto_diagnostic_group d;
+ if (warning_at (loc, OPT_Wconversion_null,
+ "passing NULL to non-pointer argument %P
of %qD",
+ argnum, fn))
+   inform (get_fndecl_argument_location (fn, argnum),
+   "  declared here");
+   }
 else
-   warning_at (loc, OPT_Wconversion_null,
-   "converting to non-pointer type %qT from
NULL", totype);
+   {
+ location_t loc
+   = expansion_point_location_if_in_system_header
(input_location);
+ warning_at (loc, OPT_Wconversion_null,
+ "converting to non-pointer type %qT from
NULL", totype);
+   }


Why is 'loc' different between the branches?


Good catch; I've consolidated them in the v3 patch.


@@ -6698,9 +6706,15 @@ conversion_null_warnings (tree totype, tree
expr, tree fn, int argnum)
   && TYPE_PTR_P (totype))
   {
 if (fn)
-   warning_at (input_location, OPT_Wconversion_null,
-   "converting % to pointer type for
argument %P "
-   "of %qD", argnum, fn);
+   {
+ location_t loc = EXPR_LOC_OR_LOC (expr, input_location);
+ auto_diagnostic_group d;
+ if (warning_at (loc, OPT_Wconversion_null,
+ "converting % to pointer type
for argument "
+ "%P of %qD", argnum, fn))
+   inform (get_fndecl_argument_location (fn, argnum),
+   "  declared here");
+   }
 else
warning_at (input_location, OPT_Wconversion_null,
"converting % to pointer type %qT",
totype);


Same question.


Likewise.


@@ -6740,6 +6754,15 @@ maybe_print_user_conv_context (conversion
*convs)
   location_t
   get_fndecl_argument_location (tree fndecl, int argnum)
   {
+  /* Gracefully fail for e.g. TEMPLATE_DECL.  */
+  if (TREE_CODE (fndecl) != FUNCTION_DECL)
+return DECL_SOURCE_LOCATION (fndecl);


For a TEMPLATE_DECL we can use DECL_TEMPLATE_RESULT or STRIP_TEMPLATE
to
get the FUNCTION_DECL.  But I'm somewhat surprised we would get here
with a TEMPLATE_DECL rather than an instantiation.


FWIW I hit this for e.g. g++.dg/diagnostic/missing-default-args.C within
the new code in check_default_args, when reporting on an template
with a param with no-default-args following a param with default args.

In the v3 patch I've removed the check for FUNCTION_DECL, in favor of
a STRIP_TEMPLATE within check_default_args.


diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c
index ffc0d0d..265826a 100644
--- a/gcc/cp/decl2.c
+++ b/gcc/cp/decl2.c
@@ -48,6 +48,7 @@ along with GCC; see the file COPYING3.  If not
see
   #include "intl.h"
   #include "c-family/c-ada-spec.h"
   #include "asan.h"
+#include "gcc-rich-location.h"
   
   /* Id for dumping the raw trees.  */

   int raw_dump_id;
@@ -5179,14 +5180,25 @@ check_default_args (tree x)
   {
 tree arg = TYPE_ARG_TYPES (TREE_TYPE (x));
 bool saw_def = false;
+  location_t loc_of_first_default_arg = UNKNOWN_LOCATION;
 int i = 0 - (TREE_CODE (TREE_TYPE (x)) == METHOD_TYPE);
 for (; arg && arg != void_list_node; arg = TREE_CHAIN (arg),
++i)
   {
 if (TREE_PURPOSE (arg))
-   saw_def = true;
+   {
+ saw_def = true;
+ location_t loc = get_fndecl_argument_location (x, i);
+ if (loc != DECL_SOURCE_LOCATION (x))
+   loc_of_first_default_arg = loc;
+   }
 else if (saw_def && !PACK_EXPANSION_P (TREE_VALUE (arg)))
{
- error ("default argument missing for parameter %P of
%q+#D", i, x);
+ location_t loc = get_fndecl_argument_location (x, i);
+ gcc_rich_location richloc (loc);
+ if (loc_of_first_default_arg != UNKNOWN_LOCATION)
+   richloc.add_range (loc_of_first_default_arg);
+ error_at (,
+   "default argument missing for parameter %P of
%q#D", i, x);


If we're going to highlight the earlier parameter that has a default
argument, we should 

[PATCH] v3: C++: improvements to diagnostics using %P (more PR c++/85110)

[David Malcolm]

On Wed, 2018-12-05 at 19:49 -0500, Jason Merrill wrote:
> On 12/3/18 5:54 PM, David Malcolm wrote:

[...]

Thanks for the review.  Here's a v3 of the patch; comments inline
below.

> > diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> > index ee099cc..cfc5641 100644
> > --- a/gcc/cp/call.c
> > +++ b/gcc/cp/call.c
> > @@ -6681,16 +6681,24 @@ conversion_null_warnings (tree totype, tree
> > expr, tree fn, int argnum)
> > if (null_node_p (expr) && TREE_CODE (totype) != BOOLEAN_TYPE
> > && ARITHMETIC_TYPE_P (totype))
> >   {
> > -  location_t loc =
> > -   expansion_point_location_if_in_system_header
> > (input_location);
> > -
> > if (fn)
> > -   warning_at (loc, OPT_Wconversion_null,
> > -   "passing NULL to non-pointer argument %P of
> > %qD",
> > -   argnum, fn);
> > +   {
> > + location_t loc = EXPR_LOC_OR_LOC (expr, input_location);
> > + loc = expansion_point_location_if_in_system_header
> > (loc);
> > + auto_diagnostic_group d;
> > + if (warning_at (loc, OPT_Wconversion_null,
> > + "passing NULL to non-pointer argument %P
> > of %qD",
> > + argnum, fn))
> > +   inform (get_fndecl_argument_location (fn, argnum),
> > +   "  declared here");
> > +   }
> > else
> > -   warning_at (loc, OPT_Wconversion_null,
> > -   "converting to non-pointer type %qT from
> > NULL", totype);
> > +   {
> > + location_t loc
> > +   = expansion_point_location_if_in_system_header
> > (input_location);
> > + warning_at (loc, OPT_Wconversion_null,
> > + "converting to non-pointer type %qT from
> > NULL", totype);
> > +   }
> 
> Why is 'loc' different between the branches?

Good catch; I've consolidated them in the v3 patch.

> > @@ -6698,9 +6706,15 @@ conversion_null_warnings (tree totype, tree
> > expr, tree fn, int argnum)
> >&& TYPE_PTR_P (totype))
> >   {
> > if (fn)
> > -   warning_at (input_location, OPT_Wconversion_null,
> > -   "converting % to pointer type for
> > argument %P "
> > -   "of %qD", argnum, fn);
> > +   {
> > + location_t loc = EXPR_LOC_OR_LOC (expr, input_location);
> > + auto_diagnostic_group d;
> > + if (warning_at (loc, OPT_Wconversion_null,
> > + "converting % to pointer type
> > for argument "
> > + "%P of %qD", argnum, fn))
> > +   inform (get_fndecl_argument_location (fn, argnum),
> > +   "  declared here");
> > +   }
> > else
> > warning_at (input_location, OPT_Wconversion_null,
> > "converting % to pointer type %qT",
> > totype);
> 
> Same question.

Likewise.

> > @@ -6740,6 +6754,15 @@ maybe_print_user_conv_context (conversion
> > *convs)
> >   location_t
> >   get_fndecl_argument_location (tree fndecl, int argnum)
> >   {
> > +  /* Gracefully fail for e.g. TEMPLATE_DECL.  */
> > +  if (TREE_CODE (fndecl) != FUNCTION_DECL)
> > +return DECL_SOURCE_LOCATION (fndecl);
> 
> For a TEMPLATE_DECL we can use DECL_TEMPLATE_RESULT or STRIP_TEMPLATE
> to 
> get the FUNCTION_DECL.  But I'm somewhat surprised we would get here 
> with a TEMPLATE_DECL rather than an instantiation.

FWIW I hit this for e.g. g++.dg/diagnostic/missing-default-args.C within
the new code in check_default_args, when reporting on an template
with a param with no-default-args following a param with default args.

In the v3 patch I've removed the check for FUNCTION_DECL, in favor of
a STRIP_TEMPLATE within check_default_args.

> > diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c
> > index ffc0d0d..265826a 100644
> > --- a/gcc/cp/decl2.c
> > +++ b/gcc/cp/decl2.c
> > @@ -48,6 +48,7 @@ along with GCC; see the file COPYING3.  If not
> > see
> >   #include "intl.h"
> >   #include "c-family/c-ada-spec.h"
> >   #include "asan.h"
> > +#include "gcc-rich-location.h"
> >   
> >   /* Id for dumping the raw trees.  */
> >   int raw_dump_id;
> > @@ -5179,14 +5180,25 @@ check_default_args (tree x)
> >   {
> > tree arg = TYPE_ARG_TYPES (TREE_TYPE (x));
> > bool saw_def = false;
> > +  location_t loc_of_first_default_arg = UNKNOWN_LOCATION;
> > int i = 0 - (TREE_CODE (TREE_TYPE (x)) == METHOD_TYPE);
> > for (; arg && arg != void_list_node; arg = TREE_CHAIN (arg),
> > ++i)
> >   {
> > if (TREE_PURPOSE (arg))
> > -   saw_def = true;
> > +   {
> > + saw_def = true;
> > + location_t loc = get_fndecl_argument_location (x, i);
> > + if (loc != DECL_SOURCE_LOCATION (x))
> > +   loc_of_first_default_arg = loc;
> > +   }
> > else if (saw_def && !PACK_EXPANSION_P (TREE_VALUE (arg)))
> > {
> > - error ("default argument missing for parameter %P of
> > %q+#D", i, x);
> > + location_t loc = get_fndecl_argument_location (x, i);
> > + gcc_rich_location richloc (loc);
> > + if (loc_of_first_default_arg != UNKNOWN_LOCATION)
> > +   richloc.add_range (loc_of_first_default_arg);
> > + 

[PATCH][RFC] Associate constant offsets next to symbol

[Richard Biener]

This massages the GIMPLE reassoc pass to associate constant offsets
into a invariant base.  For one of the testcases in PR63184 this
does

   i.0_1 = i;
   _2 = i.0_1 * 4;
   _3 = (sizetype) _2;
-  _4 = _3 + 4;
-  _5 = [1] + _4;
+  _4 = _3;
+  _5 = [(void *) + 8B] + _4;

this performs sth that RTL expansion does already with -ftree-ter.

In it's own it isn't enough to fix PR63184 thus I'm not sure it is
worth it at this point.  Without swapping SLSR and late reassoc
we still end up with

  i.0_1 = i;
  _2 = i.0_1 * 4;
  _3 = (sizetype) _2;
  _5 = [(void *) + 8B] + _3;
  _6 = i.0_1 + 2;
  _12 = (sizetype) _6;
  _13 = _3 + 8;
  _7 =  + _13;

swapping makes late rassoc perform the same trick again and then
DOM figures out the equivalency.

In the end the issue is address canonicalization, either directly
in the IL or in passes doing VN or folding.  More aggressively
using infrastructure like tree-affine would be another solution
here (but tree-affine comes at a cost so I'm worrying to use
that from say match.pd patterns).

Bootstrapped & tested on x86_64-unknown-linux-gnu.

Posting here mainly for reference.  I'll see if I can try
a tree-affine match.pd pattern tomorrow.

Richard.

2018-12-06  Richard Biener  

PR tree-optimization/63184
* tree-ssa-reassoc.c (reassociate_bb): Forward a constant
into a single-use in a POINTER_PLUS_EXPR invariant address base.

diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index a9f45bfd891..fb1f8014633 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -5988,6 +5988,31 @@ reassociate_bb (basic_block bb)
}
}
 
+ /* If the association chain is used in a single
+POINTER_PLUS_EXPR with an invariant first operand
+then combine a constant element with the invariant
+address.  */
+ use_operand_p use_p;
+ gimple *use_stmt;
+ if (ops.length () > 1
+ && rhs_code == PLUS_EXPR
+ && TREE_CODE (ops.last ()->op) == INTEGER_CST
+ && single_imm_use (lhs, _p, _stmt)
+ && is_gimple_assign (use_stmt)
+ && gimple_assign_rhs_code (use_stmt) == POINTER_PLUS_EXPR
+ && TREE_CODE (gimple_assign_rhs1 (use_stmt)) == ADDR_EXPR)
+   {
+ last = ops.pop ();
+ tree addr = gimple_assign_rhs1 (use_stmt);
+ addr = build1 (ADDR_EXPR, TREE_TYPE (addr),
+fold_build2 (MEM_REF,
+ TREE_TYPE (TREE_TYPE (addr)),
+ addr,
+ fold_convert (ptr_type_node,
+   last->op)));
+ gimple_assign_set_rhs1 (use_stmt, addr);
+   }
+
  tree new_lhs = lhs;
  /* If the operand vector is now empty, all operands were 
 consumed by the __builtin_powi optimization.  */

Re: [PATCH, OpenACC, 7/8] Multi-dimensional dynamic array support for OpenACC data clauses, libgomp support

[Jakub Jelinek]

On Thu, Dec 06, 2018 at 10:19:43PM +0800, Chung-Lin Tang wrote:
> > Why do you call the non-contiguous arrays dynamic arrays?  Is that some 
> > OpenACC term?
> > I'd also prefix those with gomp_ and it is important to make it clear what
> > is the ABI type shared with the compiler and what are the internal types.
> > struct gomp_array_descr would look more natural to me.
> 
> Well it's not particularly an OpenACC term, just that non-contiguous arrays 
> are
> often multi-dimensional arrays dynamically allocated and created through 
> (arrays of) pointers.
> Are you strongly opposed to this naming? If so, I can adjust this part.

The way how those arrays are created (and it doesn't have to be dynamically
allocated) doesn't affect their representation.
There are various terms that describe various data structures, like Iliffe
vectors, jagged/ragged arrays, dope vectors.
I guess it depends on what kind of data structures does this new framework
support, if the Iliffe vectors (arrays of pointers), or just flat but
strided arrays, etc.

> > > +  for (i = 0; i < mapnum; i++)
> > > +{
> > > +  int kind = get_kind (short_mapkind, kinds, i);
> > > +  if (GOMP_MAP_DYNAMIC_ARRAY_P (kind & typemask))
> > > + {
> > > +   da_data_row_num += gomp_dynamic_array_count_rows (hostaddrs[i]);
> > > +   da_info_num += 1;
> > > + }
> > > +}
> > 
> > I'm not really happy by adding several extra loops which will not do
> > anything in the case there are no non-contiguous arrays being mapped (for
> > now always for OpenMP (OpenMP 5 has support for non-contigious target update
> > to/from though) and guess rarely for OpenACC).
> > Can't you use some flag bit in flags passed to GOMP_target* etc. and do the
> > above loop only if the compiler indicated there are any?
> 
> I originally strived to not have that loop, but because each row in the last 
> dimension
> is mapped as its own target_var_desc, we need to count them at this stage to 
> allocate
> the right number at start. Otherwise a realloc later seems even more ugly...
> 
> We currently don't have a suitable flag word argument in GOMP_target*, 
> GOACC_parallel*, etc.
> I am not sure if such a feature warrants changing the interface.
> 
> If you are weary of OpenMP being affected, I can add a condition to restrict 
> such processing
> to only (pragma_kind == GOMP_MAP_VARS_OPENACC). Is that okay? (at least 
> before making any
> larger runtime interface adjustments)

That will still cost you doing that loop for OpenACC constructs that don't
have any of these non-contiguous arrays.  GOMP_target_ext has flags
argument, but GOACC_paralel_keyed doesn't.  It has ... and you could perhaps
encode some flag in there.  Or, could these array descriptors be passed
first in the list of vars, so instead of a loop to check for these you could
just check the first one?

> > > +  tgt = gomp_malloc (sizeof (*tgt)
> > > +  + sizeof (tgt->list[0]) * (mapnum + da_data_row_num));
> > > +  tgt->list_count = mapnum + da_data_row_num;
> > > tgt->refcount = pragma_kind == GOMP_MAP_VARS_ENTER_DATA ? 0 : 1;
> > > tgt->device_descr = devicep;
> > > struct gomp_coalesce_buf cbuf, *cbufp = NULL;
> > 
> > > @@ -687,6 +863,55 @@ gomp_map_vars (struct gomp_device_descr *devicep, 
> > > size_t mapnum,
> > >   }
> > >   }
> > > +  /* For dynamic arrays. Each data row is one target item, separated from
> > > + the normal map clause items, hence we order them after mapnum.  */
> > > +  for (i = 0, da_index = 0, row_start = 0; i < mapnum; i++)
> > 
> > Even if nothing is in flags, you could just avoid this loop if the previous
> > loop(s) haven't found any noncontiguous arrays.
> 
> I'll add a bit more checking to avoid these cases.

Jakub

Re: [PATCH, OpenACC, 7/8] Multi-dimensional dynamic array support for OpenACC data clauses, libgomp support

[Chung-Lin Tang]

Hi Jakub, thanks for the swift review a few weeks ago, and apologies I haven't 
been able
to respond sooner.

On 2018/10/16 9:13 PM, Jakub Jelinek wrote:>> +/* Dynamic array related data 
structures, interfaces with the compiler.  */

+
+struct da_dim {
+  size_t base;
+  size_t length;
+  size_t elem_size;
+  size_t is_array;
+};
+
+struct da_descr_type {
+  void *ptr;
+  size_t ndims;
+  struct da_dim dims[];
+};


Why do you call the non-contiguous arrays dynamic arrays?  Is that some OpenACC 
term?
I'd also prefix those with gomp_ and it is important to make it clear what
is the ABI type shared with the compiler and what are the internal types.
struct gomp_array_descr would look more natural to me.


Well it's not particularly an OpenACC term, just that non-contiguous arrays are
often multi-dimensional arrays dynamically allocated and created through 
(arrays of) pointers.
Are you strongly opposed to this naming? If so, I can adjust this part.

I think the suggested 'gomp_array_descr' identifier looks descriptive, I'll 
revise that in an update,
as well as add more comments to better describe its ABI significance with the 
compiler.


+  for (i = 0; i < mapnum; i++)
+{
+  int kind = get_kind (short_mapkind, kinds, i);
+  if (GOMP_MAP_DYNAMIC_ARRAY_P (kind & typemask))
+   {
+ da_data_row_num += gomp_dynamic_array_count_rows (hostaddrs[i]);
+ da_info_num += 1;
+   }
+}


I'm not really happy by adding several extra loops which will not do
anything in the case there are no non-contiguous arrays being mapped (for
now always for OpenMP (OpenMP 5 has support for non-contigious target update
to/from though) and guess rarely for OpenACC).
Can't you use some flag bit in flags passed to GOMP_target* etc. and do the
above loop only if the compiler indicated there are any?


I originally strived to not have that loop, but because each row in the last 
dimension
is mapped as its own target_var_desc, we need to count them at this stage to 
allocate
the right number at start. Otherwise a realloc later seems even more ugly...

We currently don't have a suitable flag word argument in GOMP_target*, 
GOACC_parallel*, etc.
I am not sure if such a feature warrants changing the interface.

If you are weary of OpenMP being affected, I can add a condition to restrict 
such processing
to only (pragma_kind == GOMP_MAP_VARS_OPENACC). Is that okay? (at least before 
making any
larger runtime interface adjustments)


+  tgt = gomp_malloc (sizeof (*tgt)
++ sizeof (tgt->list[0]) * (mapnum + da_data_row_num));
+  tgt->list_count = mapnum + da_data_row_num;
tgt->refcount = pragma_kind == GOMP_MAP_VARS_ENTER_DATA ? 0 : 1;
tgt->device_descr = devicep;
struct gomp_coalesce_buf cbuf, *cbufp = NULL;



@@ -687,6 +863,55 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t 
mapnum,
}
  }
  
+  /* For dynamic arrays. Each data row is one target item, separated from

+ the normal map clause items, hence we order them after mapnum.  */
+  for (i = 0, da_index = 0, row_start = 0; i < mapnum; i++)


Even if nothing is in flags, you could just avoid this loop if the previous
loop(s) haven't found any noncontiguous arrays.


I'll add a bit more checking to avoid these cases.

Thanks,
Chung-Lin

Re: [PING #4][PATCH] avoid warning on constant strncpy until next statement is reachable (PR 87028)

[Jeff Law]

On 12/6/18 6:00 AM, Christophe Lyon wrote:
> On Thu, 6 Dec 2018 at 00:11, Jeff Law  wrote:
>>
>> On 11/29/18 4:43 PM, Martin Sebor wrote:
>>> On 11/29/18 4:07 PM, Jeff Law wrote:
 On 11/29/18 1:34 PM, Martin Sebor wrote:
> On 11/16/2018 02:07 AM, Richard Biener wrote:
>> On Fri, Nov 16, 2018 at 4:12 AM Martin Sebor  wrote:
>>>
>>> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01818.html
>>>
>>> Please let me know if there is something I need to change here
>>> to make the fix acceptable or if I should stop trying.
>>
>> I have one more comment about
>>
>> +  /* Defer warning (and folding) until the next statement in the basic
>> + block is reachable.  */
>> +  if (!gimple_bb (stmt))
>> +return false;
>> +
>>
>> it's not about the next statement in the basic-block being "reachable"
>> (even w/o a CFG you can use gsi_next()) but rather that the next
>> stmt isn't yet gimplified and thus not inserted into the gimple sequence,
>>
>> right?
>
> No, it's about the current statement not being associated with
> a basic block yet when the warning code runs for the first time
> (during gimplify_expr), and so gsi_next() returning null.
>
>> You apply this to gimple_fold_builtin_strncpy but I'd rather
>> see us not sprinkling this over gimple-fold.c but instead do this
>> in gimplify.c:maybe_fold_stmt, delaying folding until say lowering.
>>
>> See the attached (untested).
>
> I would also prefer this solution.  I had tested it (in response
> to you first mentioning it back in September) and it causes quite
> a bit of fallout in tests that look for the folding to take place
> very early.  See the end of my reply here:
>
>https://gcc.gnu.org/ml/gcc-patches/2018-09/msg01248.html
>
> But I'm willing to do the test suite cleanup if you think it's
> suitable for GCC 9.  (If you're thinking GCC 10 please let me
> know now.)
 The fallout on existing tests is minimal.  What's more concerning is
 that it doesn't actually pass the new test from Martin's original
 submission.  We get bogus warnings.

 At least part of the problem is weakness in maybe_diag_stxncpy_trunc.
 It can't handle something like this:

 test_literal (char * d, struct S * s)
 {
strncpy (d, "1234567890", 3);
_1 = d + 3;
*_1 = 0;
 }


 Note the pointer arithmetic between the strncpy and storing the NUL
 terminator.
>>>
>>> Right.  I'm less concerned about this case because it involves
>>> a literal that's obviously longer than the destination but it
>>> would be nice if the suppression worked here as well in case
>>> the literal comes from macro expansion.  It will require
>>> another tweak.
>>>
>>> But the test from my patch passes with the changes to calls.c
>>> from my patch, so that's an improvement.
>>>
>>> I have done the test suite cleanup in the attached patch.  It
>>> was indeed minimal -- not sure why I saw so many failures with
>>> my initial approach.
>>>
>>> I can submit a patch to handle the literal case above as
>>> a followup unless you would prefer it done at the same time.
>>>
>>> Martin
>>>
>>> gcc-87028.diff
>>>
>>> PR tree-optimization/87028 - false positive -Wstringop-truncation strncpy 
>>> with global variable source string
>>>
>>> gcc/ChangeLog:
>>>
>>>   PR tree-optimization/87028
>>>   * calls.c (get_attr_nonstring_decl): Avoid setting *REF to
>>>   SSA_NAME_VAR.
>>>   * gcc/gimple-low.c (lower_stmt): Delay foldin built-ins.
>>>   * gimplify (maybe_fold_stmt): Avoid folding statements that
>>>   don't belong to a basic block.
>>>   * tree.h (SSA_NAME_VAR): Update comment.
>>>   * tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Simplify.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>>   PR tree-optimization/87028
>>>   * c-c++-common/Wstringop-truncation.c: Remove xfails.
>>>   * gcc.dg/Wstringop-truncation-5.c: New test.
>>>   * gcc.dg/strcmpopt_1.c: Adjust.
>>>   * gcc.dg/tree-ssa/pr79697.c: Same.
>> I fixed up the ChangeLog a little and installed the patch.
>>
> 
> Hi,
> The new test (Wstringop-truncation-5.c ) fails at least on arm and aarch64:
> FAIL: gcc.dg/Wstringop-truncation-5.c (test for excess errors)
I must have applied the hunk more than once because the contents of the
test are duplicated resulting in the errors.  I removed the duplicate
copy of the test and that should fix this problem.
jeff

CES International 2019 - Attendees List

[christin.braun]

Hi, 

This is Christin Braun, have I caught you in the middle of anything? Purpose
for my email is that I see you are Exhibiting at CES International 2019.

 I just wanted to know your interests in acquiring the Attendees/Visitors
list of CES International 2019 (The International Consumer Electronics Show)
to create more traffic to your booth in the Show.

 

Information Provided: - Company name, URL, Contact name, Job title, Phone
number, fax number, physical address, Industry, Company size, Email address.

 

We have special discounts for this month, please let me know if you are
interested. So, that I can provide you the number of Attendees and the cost.


Awaiting your reply.

 

Regards,

Christin Braun

Tradeshow Specialist

 

If you do not wish to hear from us again, please respond back with "Leave
Out" and we will honour your request.

 

Re: Add a loop versioning pass

[Richard Sandiford]

Richard Biener  writes:
>> > The pass contains an awful lot of heuristics :/  Like last year
>> > with the interchange pass I would suggest to rip most of it out
>> > and first lay infrastructure with the cases you can positively
>> > identify without applying heuristics or "hacks" like stripping
>> > semantically required casts.  That makes it also clear which
>> > testcases test which code-path.  That said, all the analyze
>> > multiplications/plusses/factors stuff was extremely hard to review
>> > and I have no overall picture why this is all so complicated or
>> > necessary.
>> 
>> I think the tests do cover most of this -- was glad to see that
>> when Kyrill tried taking something out, one of the tests started
>> to fail :-).  And the comments in the tests as well as the code
>> are supposed to explain why this is desirable.  But I can try to
>> spruce up the comments in the code if they're not detailed enough.
>> 
>> The problem is that the motivating (Fortran) case can't be identified
>> without applying heuristics.  All we see is a collection of adds and
>> multiplies, with no semantic information to say which multiplies are for
>> the inner dimension and which aren't.  I agree it would be nicer not
>> to have them, which is why I'd originally suggested the IFN to provide
>> information about dimensions.
>
> Sure, still a new pass having _all_ the heuristic built in with
> 10s of testcases do not make it easy to review those heuristics...

Fair :-)  In the patch below I've tried to cut down on the special cases
and tried to add more comments explaining which patterns the code is
trying to detect/reject.  Probably the key part is the one beginning:

+   The main difficulty here isn't finding strides that could be used
+   in a version check (that's pretty easy).  The problem instead is to
+   avoid versioning for some stride S that is unlikely ever to be 1 at
+   runtime.  Versioning for S == 1 on its own would lead to unnecessary
+   code bloat, while adding S == 1 to more realistic version conditions
+   would lose the optimisation opportunity offered by those other conditions.

>> >> +/* Return true if in principle it is worth versioning an index fragment 
>> >> of
>> >> +   the form:
>> >> +
>> >> + (i * b * SCALE) / FACTOR
>> >> +
>> >> +   for the case in which b == 1.  */
>> >> +
>> >> +bool
>> >> +loop_versioning::acceptable_scale_p (tree scale, poly_uint64 factor)
>> >> +{
>> >> +  /* See whether SCALE is a constant multiple of FACTOR, and if the
>> >> + multiple is small enough for us to treat it as a potential grouped
>> >> + access.  For example:
>> >> +
>> >> +   for (auto i : ...)
>> >> +  y[i] = f (x[4 * i * stride],
>> >> +x[4 * i * stride + 1],
>> >> +x[4 * i * stride + 2]);
>> >> +
>> >> + would benefit from versioning for the case in which stride == 1.
>> >> + High multiples of i * stride are less likely to benefit, and could
>> >> + indicate a simulated multi-dimensional array.
>> >> +
>> >> + This is just a heuristic, to avoid having to do expensive group
>> >> + analysis of the data references in a loop.  */
>> >> +  poly_uint64 const_scale;
>> >> +  unsigned int multiple;
>> >> +  if (poly_int_tree_p (scale, _scale)
>> >> +  && constant_multiple_p (const_scale, factor, ))
>> >> +{
>> >> +  unsigned int maxval = PARAM_VALUE 
>> >> (PARAM_LOOP_VERSIONING_GROUP_SIZE);
>> >> +  return IN_RANGE (multiple, 1, maxval);
>> >
>> > Hmm.  So you _do_ want to version sth like
>> >
>> > struct X { int i; int j; } a[2048];
>> >
>> > for (int i = start; i < end; ++i)
>> >   a[i*s].i = 1;
>> >
>> > ?  That is, even with s == 1 the accesses will not become contiguous?
>> > OK, I suppose that's because you are looking at a stmt in isolation
>> > and another stmt may access a[i*s].j here.
>> >
>> > That is, would it be a future improvement to run sth like the
>> > vectorizers group access analysis on the references and perform
>> > this check on whole groups then, possibly better being able to
>> > constrain what is now the magic parameter PARAM_LOOP_VERSIONING_GROUP_SIZE?
>> 
>> Yeah, possibly.  The problem is that we might end up having to reproduce
>> vectoriser heuristics.  E.g. stores with gaps should be vectorisable
>> in future with SVE, using contiguous predicated stores in which some
>> lanes are inactive.  So we don't necessarily need the .j access for
>> this to be worthwhile.
>> 
>> But perhaps the argument for versioning for vectorisation is stronger
>> for grouped accesses than contiguous ones.
>> 
>> Note that the above example doesn't rely on the grouping heuristic
>> since we just strip the COMPONENT_REF and then analyze the ARRAY_REF
>> with a factor of 1.  The heuristic instead handles things like:
>> 
>>   a[i * stride * 2]
>> 
>> etc., and the param is more there to decide when a constant factor is
>> small enough to be a realistic group size instead of an outer dimension.
>> E.g. if we see:
>> 
>>  

Re: [PING #4][PATCH] avoid warning on constant strncpy until next statement is reachable (PR 87028)

[Christophe Lyon]

On Thu, 6 Dec 2018 at 00:11, Jeff Law  wrote:
>
> On 11/29/18 4:43 PM, Martin Sebor wrote:
> > On 11/29/18 4:07 PM, Jeff Law wrote:
> >> On 11/29/18 1:34 PM, Martin Sebor wrote:
> >>> On 11/16/2018 02:07 AM, Richard Biener wrote:
>  On Fri, Nov 16, 2018 at 4:12 AM Martin Sebor  wrote:
> >
> > Ping: https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01818.html
> >
> > Please let me know if there is something I need to change here
> > to make the fix acceptable or if I should stop trying.
> 
>  I have one more comment about
> 
>  +  /* Defer warning (and folding) until the next statement in the basic
>  + block is reachable.  */
>  +  if (!gimple_bb (stmt))
>  +return false;
>  +
> 
>  it's not about the next statement in the basic-block being "reachable"
>  (even w/o a CFG you can use gsi_next()) but rather that the next
>  stmt isn't yet gimplified and thus not inserted into the gimple sequence,
> 
>  right?
> >>>
> >>> No, it's about the current statement not being associated with
> >>> a basic block yet when the warning code runs for the first time
> >>> (during gimplify_expr), and so gsi_next() returning null.
> >>>
>  You apply this to gimple_fold_builtin_strncpy but I'd rather
>  see us not sprinkling this over gimple-fold.c but instead do this
>  in gimplify.c:maybe_fold_stmt, delaying folding until say lowering.
> 
>  See the attached (untested).
> >>>
> >>> I would also prefer this solution.  I had tested it (in response
> >>> to you first mentioning it back in September) and it causes quite
> >>> a bit of fallout in tests that look for the folding to take place
> >>> very early.  See the end of my reply here:
> >>>
> >>>https://gcc.gnu.org/ml/gcc-patches/2018-09/msg01248.html
> >>>
> >>> But I'm willing to do the test suite cleanup if you think it's
> >>> suitable for GCC 9.  (If you're thinking GCC 10 please let me
> >>> know now.)
> >> The fallout on existing tests is minimal.  What's more concerning is
> >> that it doesn't actually pass the new test from Martin's original
> >> submission.  We get bogus warnings.
> >>
> >> At least part of the problem is weakness in maybe_diag_stxncpy_trunc.
> >> It can't handle something like this:
> >>
> >> test_literal (char * d, struct S * s)
> >> {
> >>strncpy (d, "1234567890", 3);
> >>_1 = d + 3;
> >>*_1 = 0;
> >> }
> >>
> >>
> >> Note the pointer arithmetic between the strncpy and storing the NUL
> >> terminator.
> >
> > Right.  I'm less concerned about this case because it involves
> > a literal that's obviously longer than the destination but it
> > would be nice if the suppression worked here as well in case
> > the literal comes from macro expansion.  It will require
> > another tweak.
> >
> > But the test from my patch passes with the changes to calls.c
> > from my patch, so that's an improvement.
> >
> > I have done the test suite cleanup in the attached patch.  It
> > was indeed minimal -- not sure why I saw so many failures with
> > my initial approach.
> >
> > I can submit a patch to handle the literal case above as
> > a followup unless you would prefer it done at the same time.
> >
> > Martin
> >
> > gcc-87028.diff
> >
> > PR tree-optimization/87028 - false positive -Wstringop-truncation strncpy 
> > with global variable source string
> >
> > gcc/ChangeLog:
> >
> >   PR tree-optimization/87028
> >   * calls.c (get_attr_nonstring_decl): Avoid setting *REF to
> >   SSA_NAME_VAR.
> >   * gcc/gimple-low.c (lower_stmt): Delay foldin built-ins.
> >   * gimplify (maybe_fold_stmt): Avoid folding statements that
> >   don't belong to a basic block.
> >   * tree.h (SSA_NAME_VAR): Update comment.
> >   * tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Simplify.
> >
> > gcc/testsuite/ChangeLog:
> >
> >   PR tree-optimization/87028
> >   * c-c++-common/Wstringop-truncation.c: Remove xfails.
> >   * gcc.dg/Wstringop-truncation-5.c: New test.
> >   * gcc.dg/strcmpopt_1.c: Adjust.
> >   * gcc.dg/tree-ssa/pr79697.c: Same.
> I fixed up the ChangeLog a little and installed the patch.
>

Hi,
The new test (Wstringop-truncation-5.c ) fails at least on arm and aarch64:
FAIL: gcc.dg/Wstringop-truncation-5.c (test for excess errors)
Excess errors:
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:74:8: error:
redefinition of 'struct S'
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:79:12: error:
redefinition of 'arr'
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:80:19: error:
redefinition of 'ptr'
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:82:12: error:
redefinition of 'arr2'
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:84:6: error:
conflicting types for 'test_literal'
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:90:6: error:
conflicting types for 'test_global_arr'
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:96:6: error:
conflicting types for 'test_global_arr2'

Re: add taishanv110 pipeline scheduling

[Kyrill Tkachov]

Hi Wu,

I notice you CC'ed the arm maintainers. This is an aarch64 patch as the arm 
(aarch32) and aarch64 ports are separate in GCC.
I've added the aarch64 maintainers on CC for you.

On 06/12/18 01:31, wuyuan (E) wrote:


Hi ARM maintainers:

The taishanv110 core uses generic pipeline scheduling, which restricted 
the performance of taishanv110 core. By adding the pipeline scheduling of 
taishanv110 core in GCC,The performance of taishanv110 has been improved.

The patch  as follows, please join.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog

old mode 100644

new mode 100755

index c4ec556..d6cf1d3

--- a/gcc/ChangeLog

+++ b/gcc/ChangeLog

@@ -1,3 +1,9 @@

+2018-12-05  wuyuan 

+

+   * config/aarch64/aarch64-cores.def: New CPU.

+   * config/aarch64/aarch64.md : Add "tsv110.md"

+   * gcc/config/aarch64/tsv110.md : pipeline description

+



No "gcc/" in the path. Also, I'd use "New file."


2018-11-26  David Malcolm 

  * dump-context.h (dump_context::dump_loc): Convert 1st param from

diff --git a/gcc/config/aarch64/aarch64-cores.def 
b/gcc/config/aarch64/aarch64-cores.def

index 74be5db..8e84844 100644

--- a/gcc/config/aarch64/aarch64-cores.def

+++ b/gcc/config/aarch64/aarch64-cores.def

@@ -99,7 +99,7 @@ AARCH64_CORE("ares", ares, cortexa57, 8_2A,  
AARCH64_FL_FOR_ARCH8_2 | AARCH64_F

/* ARMv8.4-A Architecture Processors.  */

/* HiSilicon ('H') cores. */

-AARCH64_CORE("tsv110", tsv110, cortexa57,8_4A, AARCH64_FL_FOR_ARCH8_4 
| AARCH64_FL_CRYPTO | AARCH64_FL_F16 | AARCH64_FL_AES | AARCH64_FL_SHA2, tsv110,   0x48, 
0xd01, -1)

+AARCH64_CORE("tsv110", tsv110, tsv110,8_4A, AARCH64_FL_FOR_ARCH8_4 | 
AARCH64_FL_CRYPTO | AARCH64_FL_F16 | AARCH64_FL_AES | AARCH64_FL_SHA2, tsv110,   0x48, 
0xd01, -1)

/* Qualcomm ('Q') cores. */

AARCH64_CORE("saphira", saphira, saphira,8_4A,  AARCH64_FL_FOR_ARCH8_4 
| AARCH64_FL_CRYPTO | AARCH64_FL_RCPC, saphira,   0x51, 0xC01, -1)

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md

index 82af4d4..5278d6b 100644

--- a/gcc/config/aarch64/aarch64.md

+++ b/gcc/config/aarch64/aarch64.md

@@ -348,7 +348,7 @@

(include "thunderx.md")

(include "../arm/xgene1.md")

(include "thunderx2t99.md")

-

+(include "tsv110.md")

;; ---

;; Jumps and other miscellaneous insns

;; ---

diff --git a/gcc/config/aarch64/tsv110.md b/gcc/config/aarch64/tsv110.md

new file mode 100644

index 000..e912447

--- /dev/null

+++ b/gcc/config/aarch64/tsv110.md

@@ -0,0 +1,708 @@

+;; tsv110 pipeline description

+;; Copyright (C) 2014-2016 Free Software Foundation, Inc.

+;;

+;; This file is part of GCC.

+;;

+;; GCC is free software; you can redistribute it and/or modify it

+;; under the terms of the GNU General Public License as published by

+;; the Free Software Foundation; either version 3, or (at your option)

+;; any later version.

+;;

+;; GCC is distributed in the hope that it will be useful, but

+;; WITHOUT ANY WARRANTY; without even the implied warranty of

+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

+;; General Public License for more details.

+;;

+;; You should have received a copy of the GNU General Public License

+;; along with GCC; see the file COPYING3.  If not see

+;; .

+

+(define_automaton "tsv110")

+

+(define_attr "tsv110_neon_type"

+  "neon_arith_acc, neon_arith_acc_q,

+   neon_arith_basic, neon_arith_complex,

+   neon_reduc_add_acc, neon_multiply, neon_multiply_q,

+   neon_multiply_long, neon_mla, neon_mla_q, neon_mla_long,

+   neon_sat_mla_long, neon_shift_acc, neon_shift_imm_basic,

+   neon_shift_imm_complex,

+   neon_shift_reg_basic, neon_shift_reg_basic_q, neon_shift_reg_complex,

+   neon_shift_reg_complex_q, neon_fp_negabs, neon_fp_arith,

+   neon_fp_arith_q, neon_fp_reductions_q, neon_fp_cvt_int,

+   neon_fp_cvt_int_q, neon_fp_cvt16, neon_fp_minmax, neon_fp_mul,

+   neon_fp_mul_q, neon_fp_mla, neon_fp_mla_q, neon_fp_recpe_rsqrte,

+   neon_fp_recpe_rsqrte_q, neon_fp_recps_rsqrts, neon_fp_recps_rsqrts_q,

+   neon_bitops, neon_bitops_q, neon_from_gp,

+   neon_from_gp_q, neon_move, neon_tbl3_tbl4, neon_zip_q, neon_to_gp,

+   neon_load_a, neon_load_b, neon_load_c, neon_load_d, neon_load_e,

+   neon_load_f, neon_store_a, neon_store_b, neon_store_complex,

+   unknown"

+  (cond [

+ (eq_attr "type" "neon_arith_acc, neon_reduc_add_acc,\

+ neon_reduc_add_acc_q")

+   (const_string "neon_arith_acc")

+ (eq_attr "type" "neon_arith_acc_q")

+   (const_string "neon_arith_acc_q")

+ (eq_attr "type" "neon_abs,neon_abs_q,neon_add, neon_add_q, 
neon_add_long,\

+ neon_add_widen, neon_neg, neon_neg_q,\

+ neon_reduc_add, neon_reduc_add_q,\

+ neon_reduc_add_long, neon_sub, neon_sub_q,\

+ neon_sub_long, neon_sub_widen, neon_logic,\

+ 

Re: [PATCH] Fix VRP with -fno-delete-null-pointer-checks (PR c/88367, take 2)

[Richard Biener]

On Thu, 6 Dec 2018, Jakub Jelinek wrote:

> On Thu, Dec 06, 2018 at 10:05:15AM +0100, Richard Biener wrote:
> > Note I wonder if with -fwrapv-pointer NULL automatically becomes a
> > valid address?  Or is only wrapping around half of the address
> > space UB?
> 
> Hadn't thought about -fwrapv-pointer, I guess we (especially with
> -fno-delete-null-pointer-checks) need to be even more conservative in that
> case.
> 
> Furthermore, I've discovered that the ADDR_EXPR of MEM_REF case actually
> uses get_base_address and therefore the offset on MEM_REF is just one of the
> many possible offsets in the play.
> 
> So, this patch punts for -fwrapv-pointer in some further cases, and
> adjusts the vr-values.c ADDR_EXPR handling code so that it sums up all 2 or
> 3 offsets together and looks at the resulting sign.  If
> -fdelete-null-pointer-checks -fno-wrapv-pointer, it does what it did before
> in tree-vrp.c and in vr-values.c is even more aggressive than before, as in
> even if the base pointer is varying etc., if the sum of all the offsets
> is provably non-zero, the result is non-NULL.  For
> -fno-delete-null-pointer-checks -fno-wrapv-pointer it does this only if the
> resulting offset is positive.
> 
> Does this look ok?

Little bit more expensive than before but OK.

Thanks,
Richard.

> 2018-12-06  Jakub Jelinek  
> 
>   PR c/88367
>   * tree-vrp.c (extract_range_from_binary_expr): For POINTER_PLUS_EXPR
>   with -fno-delete-null-pointer-checks, set_nonnull only if the pointer
>   is non-NULL and offset is known to have most significant bit clear.
>   * vr-values.c (vr_values::vrp_stmt_computes_nonzero): For ADDR_EXPR
>   of MEM_EXPR, return true if the MEM_EXPR has non-zero offset with
>   most significant bit clear.  If offset does have most significant bit
>   set and -fno-delete-null-pointer-checks, don't return true even if
>   the base pointer is non-NULL.
> 
>   * gcc.dg/tree-ssa/pr88367.c: New test.
> 
> --- gcc/tree-vrp.c.jj 2018-12-06 11:19:24.170939864 +0100
> +++ gcc/tree-vrp.c2018-12-06 11:50:12.104711210 +0100
> @@ -1673,9 +1673,26 @@ extract_range_from_binary_expr (value_ra
>else if (code == POINTER_PLUS_EXPR)
>   {
> /* For pointer types, we are really only interested in asserting
> -  whether the expression evaluates to non-NULL.  */
> -   if (!range_includes_zero_p ()
> -   || !range_includes_zero_p ())
> +  whether the expression evaluates to non-NULL.
> +  With -fno-delete-null-pointer-checks we need to be more
> +  conservative.  As some object might reside at address 0,
> +  then some offset could be added to it and the same offset
> +  subtracted again and the result would be NULL.
> +  E.g.
> +  static int a[12]; where [0] is NULL and
> +  ptr = [6];
> +  ptr -= 6;
> +  ptr will be NULL here, even when there is POINTER_PLUS_EXPR
> +  where the first range doesn't include zero and the second one
> +  doesn't either.  As the second operand is sizetype (unsigned),
> +  consider all ranges where the MSB could be set as possible
> +  subtractions where the result might be NULL.  */
> +   if ((!range_includes_zero_p ()
> +|| !range_includes_zero_p ())
> +   && !TYPE_OVERFLOW_WRAPS (expr_type)
> +   && (flag_delete_null_pointer_checks
> +   || (range_int_cst_p ()
> +   && !tree_int_cst_sign_bit (vr1.max ()
>   vr->set_nonnull (expr_type);
> else if (range_is_null () && range_is_null ())
>   vr->set_null (expr_type);
> --- gcc/vr-values.c.jj2018-12-06 11:19:23.550950006 +0100
> +++ gcc/vr-values.c   2018-12-06 12:59:28.26920 +0100
> @@ -297,14 +297,48 @@ vr_values::vrp_stmt_computes_nonzero (gi
>&& gimple_assign_rhs_code (stmt) == ADDR_EXPR)
>  {
>tree expr = gimple_assign_rhs1 (stmt);
> -  tree base = get_base_address (TREE_OPERAND (expr, 0));
> +  poly_int64 bitsize, bitpos;
> +  tree offset;
> +  machine_mode mode;
> +  int unsignedp, reversep, volatilep;
> +  tree base = get_inner_reference (TREE_OPERAND (expr, 0), ,
> +, , , ,
> +, );
>  
>if (base != NULL_TREE
> && TREE_CODE (base) == MEM_REF
> && TREE_CODE (TREE_OPERAND (base, 0)) == SSA_NAME)
>   {
> -   value_range *vr = get_value_range (TREE_OPERAND (base, 0));
> -   if (!range_includes_zero_p (vr))
> +   poly_offset_int off = 0;
> +   bool off_cst = false;
> +   if (offset == NULL_TREE || TREE_CODE (offset) == INTEGER_CST)
> + {
> +   off = mem_ref_offset (base);
> +   if (offset)
> + off += poly_offset_int::from (wi::to_poly_wide (offset),
> +   SIGNED);
> +   off <<= LOG2_BITS_PER_UNIT;
> + 

Re: [Patch 4/4][Aarch64] v2: Implement Aarch64 SIMD ABI

[Richard Sandiford]

Steve Ellcey  writes:
> This is a patch 4 to support the Aarch64 SIMD ABI [1] in GCC.
>
> It defines a new target hook targetm.check_part_clobbered that
> takes a rtx_insn and checks to see if it is a call to a function
> that may clobber partial registers.  It returns true by default,
> which results in the current behaviour, but if we can determine
> that the function will not do any partial clobbers (like the
> Aarch64 SIMD functions) then it returns false.

Sorry, have a feeling this is going to be at least partly going
back on what I said before, but...

The patch only really deals with one user of the part-clobbered info,
namely LRA.  And as it happens, that caller does have access to the
relevant call insns (which was a concern before), since you walk them in:

  /* Check to see if any call might do a partial clobber.  */
  partial_clobber_in_bb = false;
  FOR_BB_INSNS_REVERSE_SAFE (bb, curr_insn, next)
{
  if (CALL_P (curr_insn)
  && targetm.check_part_clobbered (curr_insn))
{
  partial_clobber_in_bb = true;
  break;
}
}

Since we're looking at the call insns anyway, we could have a hook that
"jousts" two calls and picks the one that preserves *fewer* registers.
This would mean that loop produces a single instruction that conservatively
describes the call-preserved registers.  We could then stash that
instruction in lra_reg instead of the current check_part_clobbered
boolean.

The hook should by default be a null pointer, so that we can avoid
the instruction walk on targets that don't need it.

That would mean that LRA would always have a call instruction to hand
when asking about call-preserved information.  So I think we should
add an insn parameter to targetm.hard_regno_call_part_clobbered,
with a null insn selecting the defaul behaviour.   I know it's
going to be a pain to update all callers and targets, sorry.

This would also cope with the fact that, when SVE is enabled, SIMD
functions *do* still part-clobber the registers, just in a wider mode.
The current patch doesn't handle that, and it would be hard to fix without
pessimistically treating the functions as clobbering above 64 bits
rather 128 bits.

(Really, it would be good to overhaul the whole handling of ABIs
so that we have all the information about an ABI in one structure
and can ask "what ABI does this call use"?  But that's a lot of work.
The above should be good enough as long as the call-preserved behaviour
of ABIs follows a total ordering, which it does for AArch64.)

Thanks,
Richard

[PATCH] Fix VRP with -fno-delete-null-pointer-checks (PR c/88367, take 2)

[Jakub Jelinek]

On Thu, Dec 06, 2018 at 10:05:15AM +0100, Richard Biener wrote:
> Note I wonder if with -fwrapv-pointer NULL automatically becomes a
> valid address?  Or is only wrapping around half of the address
> space UB?

Hadn't thought about -fwrapv-pointer, I guess we (especially with
-fno-delete-null-pointer-checks) need to be even more conservative in that
case.

Furthermore, I've discovered that the ADDR_EXPR of MEM_REF case actually
uses get_base_address and therefore the offset on MEM_REF is just one of the
many possible offsets in the play.

So, this patch punts for -fwrapv-pointer in some further cases, and
adjusts the vr-values.c ADDR_EXPR handling code so that it sums up all 2 or
3 offsets together and looks at the resulting sign.  If
-fdelete-null-pointer-checks -fno-wrapv-pointer, it does what it did before
in tree-vrp.c and in vr-values.c is even more aggressive than before, as in
even if the base pointer is varying etc., if the sum of all the offsets
is provably non-zero, the result is non-NULL.  For
-fno-delete-null-pointer-checks -fno-wrapv-pointer it does this only if the
resulting offset is positive.

Does this look ok?

2018-12-06  Jakub Jelinek  

PR c/88367
* tree-vrp.c (extract_range_from_binary_expr): For POINTER_PLUS_EXPR
with -fno-delete-null-pointer-checks, set_nonnull only if the pointer
is non-NULL and offset is known to have most significant bit clear.
* vr-values.c (vr_values::vrp_stmt_computes_nonzero): For ADDR_EXPR
of MEM_EXPR, return true if the MEM_EXPR has non-zero offset with
most significant bit clear.  If offset does have most significant bit
set and -fno-delete-null-pointer-checks, don't return true even if
the base pointer is non-NULL.

* gcc.dg/tree-ssa/pr88367.c: New test.

--- gcc/tree-vrp.c.jj   2018-12-06 11:19:24.170939864 +0100
+++ gcc/tree-vrp.c  2018-12-06 11:50:12.104711210 +0100
@@ -1673,9 +1673,26 @@ extract_range_from_binary_expr (value_ra
   else if (code == POINTER_PLUS_EXPR)
{
  /* For pointer types, we are really only interested in asserting
-whether the expression evaluates to non-NULL.  */
- if (!range_includes_zero_p ()
- || !range_includes_zero_p ())
+whether the expression evaluates to non-NULL.
+With -fno-delete-null-pointer-checks we need to be more
+conservative.  As some object might reside at address 0,
+then some offset could be added to it and the same offset
+subtracted again and the result would be NULL.
+E.g.
+static int a[12]; where [0] is NULL and
+ptr = [6];
+ptr -= 6;
+ptr will be NULL here, even when there is POINTER_PLUS_EXPR
+where the first range doesn't include zero and the second one
+doesn't either.  As the second operand is sizetype (unsigned),
+consider all ranges where the MSB could be set as possible
+subtractions where the result might be NULL.  */
+ if ((!range_includes_zero_p ()
+  || !range_includes_zero_p ())
+ && !TYPE_OVERFLOW_WRAPS (expr_type)
+ && (flag_delete_null_pointer_checks
+ || (range_int_cst_p ()
+ && !tree_int_cst_sign_bit (vr1.max ()
vr->set_nonnull (expr_type);
  else if (range_is_null () && range_is_null ())
vr->set_null (expr_type);
--- gcc/vr-values.c.jj  2018-12-06 11:19:23.550950006 +0100
+++ gcc/vr-values.c 2018-12-06 12:59:28.26920 +0100
@@ -297,14 +297,48 @@ vr_values::vrp_stmt_computes_nonzero (gi
   && gimple_assign_rhs_code (stmt) == ADDR_EXPR)
 {
   tree expr = gimple_assign_rhs1 (stmt);
-  tree base = get_base_address (TREE_OPERAND (expr, 0));
+  poly_int64 bitsize, bitpos;
+  tree offset;
+  machine_mode mode;
+  int unsignedp, reversep, volatilep;
+  tree base = get_inner_reference (TREE_OPERAND (expr, 0), ,
+  , , , ,
+  , );
 
   if (base != NULL_TREE
  && TREE_CODE (base) == MEM_REF
  && TREE_CODE (TREE_OPERAND (base, 0)) == SSA_NAME)
{
- value_range *vr = get_value_range (TREE_OPERAND (base, 0));
- if (!range_includes_zero_p (vr))
+ poly_offset_int off = 0;
+ bool off_cst = false;
+ if (offset == NULL_TREE || TREE_CODE (offset) == INTEGER_CST)
+   {
+ off = mem_ref_offset (base);
+ if (offset)
+   off += poly_offset_int::from (wi::to_poly_wide (offset),
+ SIGNED);
+ off <<= LOG2_BITS_PER_UNIT;
+ off += bitpos;
+ off_cst = true;
+   }
+ /* If >a is equal to X and X is ~[0, 0], the result is too.
+For -fdelete-null-pointer-checks -fno-wrapv-pointer we 

Re: [Patch 3/4][Aarch64] v2: Implement Aarch64 SIMD ABI

[Richard Sandiford]

LGTM, just minor stuff.

Steve Ellcey  writes:
> +/* Return true if the instruction is a call to a SIMD function, false
> +   if it is not a SIMD function or if we do not know anything about
> +   the function.  */
> +
> +static bool
> +aarch64_simd_call_p (rtx_insn *insn)
> +{
> +  rtx symbol;
> +  rtx call;
> +  tree fndecl;
> +
> +  if (!insn)
> +return false;

Better to arrange it so that the hook never sees null insns, since there's
nothing the hook can do in that case.  The global sets should be correct
when no other information is available.

> +/* Possibly remove some registers from register set if we know they
> +   are preserved by this call, even though they are marked as not
> +   being callee saved in CALL_USED_REGISTERS.  */
> +
> +void
> +aarch64_remove_extra_call_preserved_regs (rtx_insn *insn,
> +   HARD_REG_SET *return_set)

s/from register set/from RETURN_SET/.  But it would be better to avoid
duplicating the description of the hook so much.  Maybe:

/* Implement TARGET_REMOVE_EXTRA_CALL_PRESERVED_REGS.  If INSN calls
   a function that uses the SIMD ABI, take advantage of the extra
   call-preserved registers that the ABI provides.  */

> +{
> +  int regno;
> +
> +  if (aarch64_simd_call_p (insn))
> +{
> +  for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
> + if (FP_SIMD_SAVED_REGNUM_P (regno))
> +   CLEAR_HARD_REG_BIT (*return_set, regno);
> +}

Might as well use:

for (int regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)

> diff --git a/gcc/target.def b/gcc/target.def
> index 4b166d1..25be927 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -5757,6 +5757,12 @@ for targets that don't have partly call-clobbered 
> registers.",
>   bool, (unsigned int regno, machine_mode mode),
>   hook_bool_uint_mode_false)
>  
> +DEFHOOK
> +(remove_extra_call_preserved_regs,
> + "This hook removes some registers from the callee used register set.",

Think a bit more detail would be useful here.  E.g.:

 "This hook removes registers from the set of call-clobbered registers\n\
in @var{used_regs} if, contrary to the default rules, something guarantees\n\
that @samp{insn} preserves those registers.  For example, some targets\n\
support variant ABIs in which functions preserve more registers than\n\
normal functions would.  Removing those extra registers from @var{used_regs}\n\
can lead to better register allocation.\n\
\n\
The default implementation does nothing, which is always safe.\n\
Defining the hook is purely an optimization."

> + void, (rtx_insn *insn, HARD_REG_SET *used_regs),
> + default_remove_extra_call_preserved_regs)

You need to declare this in targhooks.h.  Please sanity-test on
something like x86 that doesn't override the hook.

> diff --git a/gcc/targhooks.c b/gcc/targhooks.c
> index 3d8b3b9..a9fb101 100644
> --- a/gcc/targhooks.c
> +++ b/gcc/targhooks.c
> @@ -2372,4 +2372,11 @@ default_speculation_safe_value (machine_mode mode 
> ATTRIBUTE_UNUSED,
>return result;
>  }
>  
> +void
> +default_remove_extra_call_preserved_regs (rtx_insn *insn ATTRIBUTE_UNUSED,
> +   HARD_REG_SET *used_regs
> + ATTRIBUTE_UNUSED)
> +{
> +}

Seems easier to leave out the parameter names and drop the ATTRIBUTE_UNUSED.
The formatting wouldn't be as awkward that way.

Thanks,
Richard

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v5]

[Nick Clifton]

Hi Ian,

  Is the patch OK with you ?

Cheers
  Nick

Re: [PATCH] Restore aarch64 support for asm ("# %a0" : : "i" (0)) (PR target/87598)

[Richard Sandiford]

Jakub Jelinek  writes:
> Hi!
>
> As mentioned in the PR, aarch64 used to allow VOIDmode CONST_INTs
> as %aN operands, but r255230 started ICEing on it and r257907 turned
> that ICE into error (output_operand_lossage).
>
> The following patch restores the previous behavior, by allowing such
> CONST_INTs through.  They will fail aarch64_classify_address a few lines
> later and so aarch64_print_address_internal will return false and either
> cause output_operand_lossage there, or if it is aarch64_print_address,
> let the generic code handle the constant.
>
> Bootstrapped/regtested on aarch64-linux on GCCFarm, ok for trunk?
>
> 2018-11-29  Jakub Jelinek  
>
>   PR target/87598
>   * config/aarch64/aarch64.c (aarch64_print_address_internal): Don't
>   call output_operand_lossage on VOIDmode CONST_INTs.  After
>   output_operand_lossage do return false.
>
>   * gcc.target/aarch64/asm-5.c: New test.
>
> --- gcc/config/aarch64/aarch64.c.jj   2018-11-26 22:21:24.891607602 +0100
> +++ gcc/config/aarch64/aarch64.c  2018-11-27 14:16:48.586358824 +0100
> @@ -7635,8 +7635,14 @@ aarch64_print_address_internal (FILE *f,
>unsigned int size;
>  
>/* Check all addresses are Pmode - including ILP32.  */
> -  if (GET_MODE (x) != Pmode)
> -output_operand_lossage ("invalid address mode");
> +  if (GET_MODE (x) != Pmode
> +  && (GET_MODE (x) != VOIDmode
> +   || !CONST_INT_P (x)
> +   || trunc_int_for_mode (INTVAL (x), Pmode) != INTVAL (x)))
> +{
> +  output_operand_lossage ("invalid address mode");
> +  return false;
> +}

The VOIDmode check is redundant, think it would be clearer without.

OK otherwise, thanks.

Richard

Re: [PATCH, Fortran] pad char to int conversions with spaces instead of zeros (legacy)

[Mark Eggleston]

On 06/12/2018 10:33, Jakub Jelinek wrote:

On Wed, Dec 05, 2018 at 06:27:00PM -0800, Jerry DeLisle wrote:

I disagree completely. I assume the idea of -fdec-pad-with-spaces is to
accomodate some old dec fortran code. The only reason to use some other
character is if someone is writing new dec fortran code, which is implying
encouraging people to be writing non standard conforming code.

I should have made this clear.


Even if it is conforming in the sense that it is processor dependent you are
encouraging people to create new non portable code across compilers. Please
just stay consistent with Intel.

I agree.

So do you prefer to always use ' ' instead of '\0', or decide based on -fdec
without a separate option controlling that?
It would be simpler to dispense with the extra option and just set space 
padding with -fdec. I think it would be odd to use something other than 
a space or '\0' for padding.


Jakub

Mark

--
https://www.codethink.co.uk/privacy.html

[C++ Patch] Three additional bitfield diagnostic tweaks (a regression fix included)

[Paolo Carlini]

Hi,

I'm bundling together 3 more. In attachment order:

1- Since we decided to explicitly print the wrong type, I suppose we 
want to consistently do that in templates too.


2- Unfortunately I have to fix another buglet I recently introduced, 
completely similar to c++/88222 fixed by Marek. Well, at least we will 
not print anymore an empty '' when the unqualified_id is null because 
the field is unnamed.


3- In the non-static case too, when from grokdeclarator we are calling 
FIELD_DECL and passing the location as first argument, I think we want 
to likewise pass declarator->id_loc when available.


The added/extended testcase exercise all of the above. Tested x86_64-linux.

Thanks, Paolo.



/cp
2018-12-06  Paolo Carlini  

* class.c (check_bitfield_decl): In error message about non-integral
type print the type itself too.
* decl.c (grokdeclarator): Do not ICE on unnamed bit-fields declared
friends; when calling build_decl for a FIELD_DECL possibly pass the
declarator->id_loc.

/testsuite
2018-12-06  Paolo Carlini  

* g++.dg/parse/bitfield7.C: New.
* g++.dg/other/bitfield2.C: Check location and type.
* g++.dg/parse/bitfield1.C: Likewise.
* g++.dg/parse/bitfield2.C: Likewise.
Index: cp/class.c
===
--- cp/class.c  (revision 266840)
+++ cp/class.c  (working copy)
@@ -3218,7 +3218,8 @@ check_bitfield_decl (tree field)
   /* Detect invalid bit-field type.  */
   if (!INTEGRAL_OR_ENUMERATION_TYPE_P (type))
 {
-  error ("bit-field %q+#D with non-integral type", field);
+  error_at (DECL_SOURCE_LOCATION (field),
+   "bit-field %q#D with non-integral type %qT", field, type);
   w = error_mark_node;
 }
   else
Index: cp/decl.c
===
--- cp/decl.c   (revision 266840)
+++ cp/decl.c   (working copy)
@@ -12446,9 +12446,13 @@ grokdeclarator (const cp_declarator *declarator,
  {
if (friendp)
  {
-   error_at (declarator->id_loc,
- "%qE is neither function nor member function; "
- "cannot be declared friend", unqualified_id);
+   if (unqualified_id && declarator)
+ error_at (declarator->id_loc,
+   "%qE is neither function nor member function; "
+   "cannot be declared friend", unqualified_id);
+   else
+ error ("unnamed field is neither function nor member "
+"function; cannot be declared friend");
return error_mark_node;
  }
decl = NULL_TREE;
@@ -12536,7 +12540,9 @@ grokdeclarator (const cp_declarator *declarator,
  unqualified_id);
constexpr_p = false;
  }
-   decl = build_decl (input_location,
+   decl = build_decl (declarator
+  ? declarator->id_loc
+  : input_location,
   FIELD_DECL, unqualified_id, type);
DECL_NONADDRESSABLE_P (decl) = bitfield;
if (bitfield && !unqualified_id)
Index: testsuite/g++.dg/other/bitfield2.C
===
--- testsuite/g++.dg/other/bitfield2.C  (revision 266840)
+++ testsuite/g++.dg/other/bitfield2.C  (working copy)
@@ -3,7 +3,7 @@
 
 struct A
 {
-  double d : 2;  // { dg-error "non-integral" }
+  double d : 2;  // { dg-error "10:bit-field .d. with non-integral type 
.double." }
   A() {}
   ~A() {}
 };
Index: testsuite/g++.dg/parse/bitfield1.C
===
--- testsuite/g++.dg/parse/bitfield1.C  (revision 266840)
+++ testsuite/g++.dg/parse/bitfield1.C  (working copy)
@@ -2,7 +2,7 @@
 
 struct A
 {
-  double i : 8; // { dg-error "type" }
+  double i : 8; // { dg-error "10:bit-field .i. with non-integral type 
.double." }
 };
 
 void foo(A& a)
Index: testsuite/g++.dg/parse/bitfield2.C
===
--- testsuite/g++.dg/parse/bitfield2.C  (revision 266840)
+++ testsuite/g++.dg/parse/bitfield2.C  (working copy)
@@ -4,7 +4,7 @@ struct X {};
 
 struct A
 {
-X x : 2;// { dg-error "non-integral type" }
+X x : 2;// { dg-error "7:bit-field .x. with non-integral type 
.X." }
 };
 struct B : A {};
 
@@ -19,7 +19,7 @@ C c;
 template 
 struct D
 {
-  T t : 3;  // { dg-error "non-integral type" }
+  T t : 3;  // { dg-error "5:bit-field .double D\\::t. 
with non-integral type .double." }
 };
 
 D d;// { dg-message "required" }
@@ -28,7 +28,7 @@ template 
 struct E
 {
   typedef T* U;
-  U t : 3; // { dg-error "non-integral type" }
+  U t : 3;

Re: [PATCH, Fortran] pad char to int conversions with spaces instead of zeros (legacy)

[Jakub Jelinek]

On Wed, Dec 05, 2018 at 06:27:00PM -0800, Jerry DeLisle wrote:
> I disagree completely. I assume the idea of -fdec-pad-with-spaces is to
> accomodate some old dec fortran code. The only reason to use some other
> character is if someone is writing new dec fortran code, which is implying
> encouraging people to be writing non standard conforming code.
> 
> Even if it is conforming in the sense that it is processor dependent you are
> encouraging people to create new non portable code across compilers. Please
> just stay consistent with Intel.

So do you prefer to always use ' ' instead of '\0', or decide based on -fdec
without a separate option controlling that?

Jakub

Re: [PATCH, Fortran] pad char to int conversions with spaces instead of zeros (legacy)

[Mark Eggleston]

On 04/12/2018 17:04, Fritz Reese wrote:

On Tue, Dec 4, 2018 at 10:12 AM Jakub Jelinek  wrote:

Just a couple of random comments.
-fdec-pad-with-spaces option name doesn't look right, because it doesn't say
what the option affects.  So perhaps have transfer in the option name?

[...]

Wouldn't it be better to allow specifying whatever character you want to pad
with, so that users can choose something even different?

I concur with this. In that case some option like -ftransfer-pad-char=
may be a more appriopriate name, where -fdec may enable a default
transfer-pad-char of \x20 rather than \x00.


How would the value be specified?

-ftransfer-pad-char=0x20
-ftransfer-pad-char=32
-ftransfer-pad-char=' '

If -fdec and -ftransfer-pad-char are both specified which should have 
precedence?


For -fdec and -fno-dec both specified would I be correct in assuming 0x0 
should be used?





--- a/gcc/fortran/simplify.c
+++ b/gcc/fortran/simplify.c
@@ -7830,7 +7830,7 @@ gfc_simplify_transfer (gfc_expr *source, gfc_expr *mold, 
gfc_expr *size)

[...]

This affects just the simplification when the argument is a known constant.
Shouldn't we handle it also when it is not a constant?

Yes. Mark, you'll need to also patch iresolve.c (gfc_resolve_transfer)
to affect non-constant resolution.

Thanks for the hint.



The tests look too much big-endian vs. little-endian dependent and
ascii dependent.  We have pdp-endian and doesn't s390x TPF use EBCDIC?

Wouldn't it be better to compare transfer("ABCE", 1_8) with transfer("ABCE
", 1_8)

Agreed.

---
Fritz


Thanks for the feedback.

Mark

--
https://www.codethink.co.uk/privacy.html

[PATCH] Adjust PR63184 testcases

[Richard Biener]

Committed.

2018-12-06  Richard Biener  

PR middle-end/63184
* c-c++-common/pr19807-2.c: Try link only on x86, add xfailed
optimized dump scanning.
* c-c++-common/pr19807-3.c: Likewise.

diff --git a/gcc/testsuite/c-c++-common/pr19807-2.c 
b/gcc/testsuite/c-c++-common/pr19807-2.c
index c8b2a57d654..d2c010140d0 100644
--- a/gcc/testsuite/c-c++-common/pr19807-2.c
+++ b/gcc/testsuite/c-c++-common/pr19807-2.c
@@ -1,5 +1,6 @@
-/* { dg-do link } */
-/* { dg-options "-O" } */
+/* Some targets can optimize this on RTL.  */
+/* { dg-do link { target { x86_64-*-* i?86-*-* } } } */
+/* { dg-options "-O -fdump-tree-optimized" } */
 
 extern void link_error(void);
 int i;
@@ -10,3 +11,5 @@ int main()
 link_error();
   return 0;
 }
+
+/* { dg-final { scan-tree-dump-not "link_error" "optimized" { xfail *-*-* } } 
} */
diff --git a/gcc/testsuite/c-c++-common/pr19807-3.c 
b/gcc/testsuite/c-c++-common/pr19807-3.c
index d882bd369bf..bb7f9827725 100644
--- a/gcc/testsuite/c-c++-common/pr19807-3.c
+++ b/gcc/testsuite/c-c++-common/pr19807-3.c
@@ -1,5 +1,6 @@
-/* { dg-do link } */
-/* { dg-options "-O" } */
+/* Some targets can optimize this on RTL.  */
+/* { dg-do link { target { x86_64-*-* i?86-*-* } } } */
+/* { dg-options "-O -fdump-tree-optimized" } */
 
 extern void link_error(void);
 int i;
@@ -10,3 +11,5 @@ int main()
 link_error();
   return 0;
 }
+
+/* { dg-final { scan-tree-dump-not "link_error" "optimized" { xfail *-*-* } } 
} */

Re: [PATCH, Fortran] pad char to int conversions with spaces instead of zeros (legacy)

[Mark Eggleston]

On 04/12/2018 15:11, Jakub Jelinek wrote:

On Tue, Dec 04, 2018 at 02:47:25PM +, Mark Eggleston wrote:

Here is a patch to considered for incorporation into gfortran adding to its
legacy support. It pads character to integer conversions using spaces
instead of zeros when enabled.

The pad character is 'undefined' or 'processor dependent' depending on which
standard you read. This makes it 0x20 which matches the Oracle Fortran
compiler.

Trying fortran.godbolt.org, I think ifort pads this with spaces too.


Enabled using -fdec-pad-with-spaces and -fdec.

Just a couple of random comments.
-fdec-pad-with-spaces option name doesn't look right, because it doesn't say
what the option affects.  So perhaps have transfer in the option name?
Wouldn't it be better to allow specifying whatever character you want to pad
with, so that users can choose something even different?
Fritz Reese agrees with you here and suggests -ftransfer-pad-char= with 
-fdec setting it to 0x20.



--- a/gcc/fortran/simplify.c
+++ b/gcc/fortran/simplify.c
@@ -7830,7 +7830,7 @@ gfc_simplify_transfer (gfc_expr *source, gfc_expr *mold, 
gfc_expr *size)
/* Allocate the buffer to store the binary version of the source.  */
buffer_size = MAX (source_size, result_size);
buffer = (unsigned char*)alloca (buffer_size);
-  memset (buffer, 0, buffer_size);
+  memset (buffer, (flag_dec_pad_with_spaces ? 0x20 : 0x0), buffer_size);

This affects just the simplification when the argument is a known constant.
Shouldn't we handle it also when it is not a constant?  Like I've tried:
program test
   integer(kind=8) :: a, b, c
   character(len=4) :: e
   a = transfer("ABCE", 1_8)
   e = "ABCE"
   b = transfer(e, 1_8)
   c = transfer("ABCE", 1_8)
   print *, a, b, c
end
and for a the result is on little-endian indeed z'45434241', for b
the upper 32 bits are completely random:
 D.3854 = 4;
 D.3856 = 8;
 _1 = MIN_EXPR ;
 _2 = MAX_EXPR <_1, 0>;
 _3 = (unsigned long) _2;
 __builtin_memcpy (, , _3);
 transfer.3_4 = transfer.0;
 b = transfer.3_4;
and for c it is the padding with zeros I assume you want for -fdec.
Clearly insufficient testing let this through. The padding should be 
done for both literals and variables.

So, what does Oracle fortran or ifort do for this b case above?
Don't have access to either of those compilers. It may be possible to 
check the ifort compiler.



--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/dec-pad-with-spaces-1.f90
@@ -0,0 +1,17 @@
+! { dg-do run }
+! { dg-options "-fdec-pad-with-spaces" }
+!
+! Test case contributed by Mark Eggleston 
+
+program test
+  integer(kind=8) :: a
+  a = transfer("ABCE", 1_8)
+  ! If a has not been converted into big endian
+  ! or little endian integer it has failed.
+  if ((a.ne.int(z'4142434520202020',kind=8)).and. &
+  (a.ne.int(z'2020202045434241',kind=8))) then

The tests look too much big-endian vs. little-endian dependent and
ascii dependent.  We have pdp-endian and doesn't s390x TPF use EBCDIC?

I hadn't considered those.


Wouldn't it be better to compare transfer("ABCE", 1_8) with transfer("ABCE
", 1_8)
?

That simplifies things.


Jakub


Thanks for the feedback. I inherited this patch, my addition of 
-fdec-pad-with-spaces and improved testing were insufficient.


Will resubmit the patch after taking these issues into account.

Mark.

--
https://www.codethink.co.uk/privacy.html

Re: [PATCH][RFC] Poison bitmap_head->obstack

[Richard Biener]

On Wed, 5 Dec 2018, Jeff Law wrote:

> On 12/5/18 7:58 AM, Richard Biener wrote:
> > On Wed, 5 Dec 2018, Jeff Law wrote:
> > 
> >> On 12/4/18 6:16 AM, Richard Biener wrote:
> >>>
> >>> This tries to make bugs like that in PR88317 harder to create by
> >>> introducing a bitmap_release function that can be used as
> >>> pendant to bitmap_initialize for non-allocated bitmap heads.
> >>> The function makes sure to poison the bitmaps obstack member
> >>> so the obstack the bitmap was initialized with can be safely
> >>> released.
> >>>
> >>> The patch also adds a default constructor to bitmap_head
> >>> doing the same, but for C++ reason initializes to a
> >>> all-zero bitmap_obstack rather than 0xdeadbeef because
> >>> the latter isn't possible in constexpr context (it is
> >>> by using unions but then things start to look even more ugly).
> >>>
> >>> The stage1 compiler might end up with a few extra runtime
> >>> initializers but constexpr makes sure they'll vanish for
> >>> later stages.
> >>>
> >>> I had to paper over that you-may-not-use-memset-to-zero classes
> >>> with non-trivial constructors warning in two places and I
> >>> had to teach gengtype about CONSTEXPR (probably did so in
> >>> an awkward way - suggestions and pointers into gengtype
> >>> appreciated).
> >>>
> >>> Bootstrapped (with host GCC 4.8) on x86_64-unknown-linux-gnu,
> >>> testing in progress.
> >>>
> >>> The LRA issue seems to be rare enough (on x86_64...) that
> >>> I didn't trip over it sofar.
> >>>
> >>> Comments?  Do we want this?  Not sure how we can easily
> >>> discover all bitmap_clear () users that should really
> >>> use bitmap_release (suggestion for a better name appreciated
> >>> as well - I thought about bitmap_uninitialize)
> >>>
> >>> Richard.
> >>>
> >>> 2018-12-04  Richard Biener  
> >>>
> >>>   * bitmap.c (bitmap_head::crashme): Define.
> >>>   * bitmap.h (bitmap_head): Add constexpr default constructor
> >>>   poisoning the obstack member.
> >>>   (bitmap_head::crashme): Declare.
> >>>   (bitmap_release): New function clearing a bitmap and poisoning
> >>>   the obstack member.
> >>>   * gengtype.c (main): Make it recognize CONSTEXPR.
> >>>
> >>>   * lra-constraints.c (lra_inheritance): Use bitmap_release
> >>>   instead of bitmap_clear.
> >>>
> >>>   * ira.c (ira): Work around warning.
> >>>   * regrename.c (create_new_chain): Likewise.
> >> I don't see enough complexity in here to be concerning -- so if it makes
> >> it harder to make mistakes, then I'm for it.
> > 
> > Any comment about the -Wclass-memaccess workaround sprinkling around two
> > (void *) conversions?  I didn't dig deep enough to look for a more
> > appropriate solution, also because there were some issues with older
> > host compilers and workarounds we installed elsewhere...
> Not really.  It was just a couple casts and a normal looking ctor, so it
> didn't seem terrible.  Someone with more C++-fu may have a better
> suggestion, but it seemed reasonable to me.
> 
> > 
> > Otherwise yes, it makes it harder to do mistakes.  I'll probably
> > use bitmap_head::crashme instead of 0xdeadbeef in bitmap_release.
> > And of course we'd need to hunt down users of bitmap_clear that
> > should be bitmap_release instead...
> Right, but when we trip this kind of thing we'll know to starting
> digging around the bitmap_clear calls :-)  That's a huge head start.

OK.  I'll commit the patch later today then.

Currently testing with the following followup after I adjusted
all 'static bitmap_head ' vars in gcc/*.c.  I noticed some
oddities there, like using GC allocation for such bitmaps but
the bitmaps being not marked GTY (and being short-lived), and
sel-sched.c exporting a bitmap_head via a pointer, not using
the bitmap_head directly anywhere.

Bootstrap & regtest running on x86_64-unknown-linux-gnu.

>From 6c90c1c10f0f91a7a37feadd4f583ed8aaf5bcc7 Mon Sep 17 00:00:00 2001
From: Richard Guenther 
Date: Thu, 6 Dec 2018 10:28:30 +0100
Subject: [PATCH] bitmap-poison-followup

2018-12-06  Richard Biener  

* df-problems.c (df_rd_local_compute): Use bitmap_release.
(df_live_free): Likewise.
(df_md_local_compute): Likewise.
(df_md_free): Release df_md_scratch bitmap.
* loop-invariant.c (calculate_loop_reg_pressure): Use
bitmap_release.
* sched-deps.c (true_dependency_cache, output_dependency_cache,
anti_dependency_cache, control_dependency_cache,
spec_dependency_cache): Use bitmap instead of bitmap_head *.
* sched-ebb.c (schedule_ebbs_init): Initialize non-GTY
dont_calc_deps as bitmap allocated from obstack not GC.
(schedule_ebbs_finish): Use bitmap_release.
* sched-rgn.c (schedule_insns): Initialize non-GTY
not_in_df as bitmap allocated from obstack not GC.
Use bitmap_release.
* sel-sched.c (_forced_ebb_heads): Remove premature optimization.
(sel_region_init): Allocate forced_ebb_heads.
(sel_region_finish): Free 

Re: [PATCH] Fix VRP with -fno-delete-null-pointer-checks (PR c/88367)

[Richard Biener]

On Thu, 6 Dec 2018, Jakub Jelinek wrote:

> Hi!
> 
> If we consider -fno-delete-null-pointer-checks as a way to support e.g. AVR
> and other targets which can validly place objects at NULL rather than a way
> to workaround UBs in code, I believe the following testcase must pass if
> there is e.g.
> char a[32];   // And this object ends up at address 0
> void bar (void);
> int main () { foo ([3]); baz ([6]); }
> but fails right now.  As mentioned in the PR, in GCC 8 we used to do:
>   else if (code == POINTER_PLUS_EXPR)
> {
>   /* For pointer types, we are really only interested in asserting
>  whether the expression evaluates to non-NULL.  */
>   if (range_is_nonnull () || range_is_nonnull ())
> set_value_range_to_nonnull (vr, expr_type);
> and that triggered pretty much never, as range_is_nonnull requires that the
> offset is ~[0, 0] exactly, e.g. if it is a constant, it is never that way,
> but now we do:
> if (!range_includes_zero_p () || !range_includes_zero_p ())
> which is e.g. always if the offset is constant non-zero.
> 
> I hope we can still say that pointer wrapping even with
> -fno-delete-null-pointer-checks is UB, so this patch differentiates between
> positive offsets (in ssizetype), negative offsets (in ssizetype) and zero
> offsets and handles both the same for both ptr p+ offset and _REF[ptr, 
> offset]
> If offset is 0 and ptr is ~[0, 0], then the result is ~[0, 0] as before.
> If offset is positive in ssizetype, then even for VARYING ptr the result is
> ~[0, 0] pointer.  If the offset is (or maybe could be) negative in
> ssizetype, then for -fno-delete-null-pointer-checks the result is VARYING,
> as we could go from a non-NULL pointer back to NULL on those targets; for
> -fdelete-null-pointer-checks we do what we've done before, i.e. ~[0, 0].
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

Note I wonder if with -fwrapv-pointer NULL automatically becomes a
valid address?  Or is only wrapping around half of the address
space UB?

Richard.

> 2018-12-06  Jakub Jelinek  
> 
>   PR c/88367
>   * tree-vrp.c (extract_range_from_binary_expr): For POINTER_PLUS_EXPR
>   with -fno-delete-null-pointer-checks, set_nonnull only if the pointer
>   is non-NULL and offset is known to have most significant bit clear.
>   * vr-values.c (vr_values::vrp_stmt_computes_nonzero): For ADDR_EXPR
>   of MEM_EXPR, return true if the MEM_EXPR has non-zero offset with
>   most significant bit clear.  If offset does have most significant bit
>   set and -fno-delete-null-pointer-checks, don't return true even if
>   the base pointer is non-NULL.
> 
>   * gcc.dg/tree-ssa/pr88367.c: New test.
> 
> --- gcc/tree-vrp.c.jj 2018-12-04 13:00:02.408635579 +0100
> +++ gcc/tree-vrp.c2018-12-05 19:07:36.187567781 +0100
> @@ -1673,9 +1673,25 @@ extract_range_from_binary_expr (value_ra
>else if (code == POINTER_PLUS_EXPR)
>   {
> /* For pointer types, we are really only interested in asserting
> -  whether the expression evaluates to non-NULL.  */
> -   if (!range_includes_zero_p ()
> -   || !range_includes_zero_p ())
> +  whether the expression evaluates to non-NULL.
> +  With -fno-delete-null-pointer-checks we need to be more
> +  conservative.  As some object might reside at address 0,
> +  then some offset could be added to it and the same offset
> +  subtracted again and the result would be NULL.
> +  E.g.
> +  static int a[12]; where [0] is NULL and
> +  ptr = [6];
> +  ptr -= 6;
> +  ptr will be NULL here, even when there is POINTER_PLUS_EXPR
> +  where the first range doesn't include zero and the second one
> +  doesn't either.  As the second operand is sizetype (unsigned),
> +  consider all ranges where the MSB could be set as possible
> +  subtractions where the result might be NULL.  */
> +   if ((!range_includes_zero_p ()
> +|| !range_includes_zero_p ())
> +   && (flag_delete_null_pointer_checks
> +   || (range_int_cst_p ()
> +   && !tree_int_cst_sign_bit (vr1.max ()
>   vr->set_nonnull (expr_type);
> else if (range_is_null () && range_is_null ())
>   vr->set_null (expr_type);
> --- gcc/vr-values.c.jj2018-11-29 08:41:33.152749436 +0100
> +++ gcc/vr-values.c   2018-12-05 19:37:56.222582823 +0100
> @@ -303,8 +303,17 @@ vr_values::vrp_stmt_computes_nonzero (gi
> && TREE_CODE (base) == MEM_REF
> && TREE_CODE (TREE_OPERAND (base, 0)) == SSA_NAME)
>   {
> -   value_range *vr = get_value_range (TREE_OPERAND (base, 0));
> -   if (!range_includes_zero_p (vr))
> +   if (integer_zerop (TREE_OPERAND (base, 1))
> +   || flag_delete_null_pointer_checks)
> + {
> +   value_range *vr = get_value_range 

Re: [PATCH] Don't optimize successive divs if there is also a mod with the same last arg (PR tree-optimization/85726)

[Richard Biener]

On Thu, 6 Dec 2018, Jakub Jelinek wrote:

> Hi!
> 
> This is my proposal for fixing this PR, just a heuristics when optimizing
> successive divides might not be a good idea (first testcase), and includes
> Marc's testcases which showed cases where optimizing successive divides is a
> good idea even if the inner divide is not a single use.
> 
> Unfortunately match.pd doesn't allow to capture the outermost expression, so
> I can't narrow it even more by checking if the outer divide is in the same
> bb as the modulo.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

+  if (!gimple_in_ssa_p (cfun) || TREE_CODE (inner_div) != SSA_NAME)
+return false;

I think the latter is always true.  The code handles the case of
missing immediate uses by performing the folding which is probably
OK but a general "issue" of using stmt operands or immediate uses
in GIMPLE folding patterns.

I think the patch is OK but I also hope we do not add too many
of these heuristics...

Richard.

> 2018-12-06  Jakub Jelinek  
> 
>   PR tree-optimization/85726
>   * generic-match-head.c (optimize_successive_divisions_p): New function.
>   * gimple-match-head.c (optimize_successive_divisions_p): Likewise.
>   * match.pd: Don't combine successive divisions if they aren't exact
>   and optimize_successive_divisions_p is false.
> 
>   * gcc.dg/tree-ssa/pr85726-1.c: New test.
>   * gcc.dg/tree-ssa/pr85726-2.c: New test.
>   * gcc.dg/tree-ssa/pr85726-3.c: New test.
>   * gcc.dg/tree-ssa/pr85726-4.c: New test.
> 
> --- gcc/generic-match-head.c.jj   2018-03-28 21:14:28.124743854 +0200
> +++ gcc/generic-match-head.c  2018-12-05 16:07:43.710801347 +0100
> @@ -77,3 +77,12 @@ canonicalize_math_after_vectorization_p
>  {
>return false;
>  }
> +
> +/* Return true if successive divisions can be optimized.
> +   Defer to GIMPLE opts.  */
> +
> +static inline bool
> +optimize_successive_divisions_p (tree, tree)
> +{
> +  return false;
> +}
> --- gcc/gimple-match-head.c.jj2018-10-10 10:50:55.812109572 +0200
> +++ gcc/gimple-match-head.c   2018-12-05 16:39:50.358122589 +0100
> @@ -1163,3 +1163,27 @@ optimize_pow_to_exp (tree arg0, tree arg
>  return false;
>return true;
>  }
> +
> +/* Return true if a division INNER_DIV / DIVISOR where INNER_DIV
> +   is another division can be optimized.  Don't optimize if INNER_DIV
> +   is used in a TRUNC_MOD_EXPR with DIVISOR as second operand.  */
> +
> +static bool
> +optimize_successive_divisions_p (tree divisor, tree inner_div)
> +{
> +  if (!gimple_in_ssa_p (cfun) || TREE_CODE (inner_div) != SSA_NAME)
> +return false;
> +
> +  imm_use_iterator imm_iter;
> +  use_operand_p use_p;
> +  FOR_EACH_IMM_USE_FAST (use_p, imm_iter, inner_div)
> +{
> +  gimple *use_stmt = USE_STMT (use_p);
> +  if (!is_gimple_assign (use_stmt)
> +   || gimple_assign_rhs_code (use_stmt) != TRUNC_MOD_EXPR
> +   || !operand_equal_p (gimple_assign_rhs2 (use_stmt), divisor, 0))
> + continue;
> +  return false;
> +}
> +  return true;
> +}
> --- gcc/match.pd.jj   2018-11-30 21:36:22.273762329 +0100
> +++ gcc/match.pd  2018-12-05 16:47:27.798596450 +0100
> @@ -312,17 +312,19 @@ (define_operator_list COND_TERNARY
> and floor_div is trickier and combining round_div even more so.  */
>  (for div (trunc_div exact_div)
>   (simplify
> -  (div (div @0 INTEGER_CST@1) INTEGER_CST@2)
> +  (div (div@3 @0 INTEGER_CST@1) INTEGER_CST@2)
>(with {
>  wi::overflow_type overflow;
>  wide_int mul = wi::mul (wi::to_wide (@1), wi::to_wide (@2),
>   TYPE_SIGN (type), );
> }
> -   (if (!overflow)
> -(div @0 { wide_int_to_tree (type, mul); })
> -(if (TYPE_UNSIGNED (type)
> -  || mul != wi::min_value (TYPE_PRECISION (type), SIGNED))
> - { build_zero_cst (type); })
> +   (if (div == EXACT_DIV_EXPR
> + || optimize_successive_divisions_p (@2, @3))
> +(if (!overflow)
> + (div @0 { wide_int_to_tree (type, mul); })
> + (if (TYPE_UNSIGNED (type)
> +   || mul != wi::min_value (TYPE_PRECISION (type), SIGNED))
> +  { build_zero_cst (type); }))
>  
>  /* Combine successive multiplications.  Similar to above, but handling
> overflow is different.  */
> --- gcc/testsuite/gcc.dg/tree-ssa/pr85726-1.c.jj  2018-12-05 
> 16:55:24.852680964 +0100
> +++ gcc/testsuite/gcc.dg/tree-ssa/pr85726-1.c 2018-12-05 16:50:14.489853926 
> +0100
> @@ -0,0 +1,19 @@
> +/* PR tree-optimization/85726 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +/* { dg-final { scan-tree-dump " / 16;" "optimized" } } */
> +/* { dg-final { scan-tree-dump " / 3;" "optimized" } } */
> +/* { dg-final { scan-tree-dump " % 3;" "optimized" } } */
> +/* { dg-final { scan-tree-dump-not " / 48;" "optimized" } } */
> +
> +int ww, vv;
> +
> +int
> +foo (int y)
> +{
> +  int z = y / 16;
> +  int w = z / 3;
> +  int v = z % 3;
> +  ww = w;
> +  return v;
> +}
> --- 

Re: [PATCH] Handle clobber stmts in convert_nonlocal_reference_stmt (PR fortran/88304)

[Richard Biener]

On Thu, 6 Dec 2018, Jakub Jelinek wrote:

> Hi!
> 
> The following patch handles clobber stmts the same how we handle them in
> convert_local_reference_stmt, if it is the clobber with decl on the lhs and
> that lhs needs to be replaced by a field inside of a structure, the clobber
> is replaced by GIMPLE_NOP.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

Richard.

> 2018-12-06  Jakub Jelinek  
> 
>   PR fortran/88304
>   * tree-nested.c (convert_nonlocal_reference_stmt): Remove clobbers
>   for non-local automatic decls.
> 
>   * gfortran.fortran-torture/compile/pr88304.f90: New test.
> 
> --- gcc/tree-nested.c.jj  2018-12-02 13:50:20.186440444 +0100
> +++ gcc/tree-nested.c 2018-12-05 11:28:54.979178773 +0100
> @@ -1648,6 +1648,21 @@ convert_nonlocal_reference_stmt (gimple_
>*handled_ops_p = false;
>return NULL_TREE;
>  
> +case GIMPLE_ASSIGN:
> +  if (gimple_clobber_p (stmt))
> + {
> +   tree lhs = gimple_assign_lhs (stmt);
> +   if (DECL_P (lhs)
> +   && !(TREE_STATIC (lhs) || DECL_EXTERNAL (lhs))
> +   && decl_function_context (lhs) != info->context)
> + {
> +   gsi_replace (gsi, gimple_build_nop (), true);
> +   break;
> + }
> + }
> +  *handled_ops_p = false;
> +  return NULL_TREE;
> +
>  default:
>/* For every other statement that we are not interested in
>handling here, let the walker traverse the operands.  */
> --- gcc/testsuite/gfortran.fortran-torture/compile/pr88304.f90.jj 
> 2018-12-05 11:31:04.232046102 +0100
> +++ gcc/testsuite/gfortran.fortran-torture/compile/pr88304.f90
> 2018-12-05 11:30:57.092163910 +0100
> @@ -0,0 +1,24 @@
> +! PR fortran/88304
> +
> +module pr88304
> +  implicit none
> +  type t
> + integer :: b = -1
> +  end type t
> +contains
> +  subroutine f1 (x, y)
> +integer, intent(out) :: x, y
> +x = 5
> +y = 6
> +  end subroutine f1
> +  subroutine f2 ()
> +type(t) :: x
> +integer :: y
> +call f3
> +if (x%b .ne. 5 .or. y .ne. 6) stop 1
> +  contains
> +subroutine f3
> +  call f1 (x%b, y)
> +end subroutine f3
> +  end subroutine f2
> +end module pr88304
> 
>   Jakub
> 
> 

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

Re: [patch,openacc] Propagate independent clause for OpenACC kernels pass

[Richard Biener]

On Wed, 5 Dec 2018, Julian Brown wrote:

> On Tue, 4 Dec 2018 14:55:03 +0100
> Richard Biener  wrote:
> 
> > On Tue, 4 Dec 2018, Jakub Jelinek wrote:
> > 
> > > On Mon, Dec 03, 2018 at 11:40:39PM +, Julian Brown wrote:  
> > > > Jakub asked in the following email at the time of the patch
> > > > submission for the gomp4 branch what the difference was between
> > > > the new marked_independent flag and safelen == INT_MAX:
> > > > 
> > > >   https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01100.html
> > > > 
> > > > If I understand the followup correctly,
> > > > 
> > > >   https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01117.html
> > > > 
> > > > a setting of safelen > 1 means that up to that number of loop
> > > > iterations can run together in lockstep (as if each insn in the
> > > > loop was blindly rewritten to a safelen-width SIMD equivalent) --
> > > > but anything that happens in iteration N + 1 cannot happen before
> > > > something that happens in iteration N. Chung-Lin pointed out that
> > > > OpenACC's semantics are even less strict (allowing iterations to
> > > > proceed fully independently in an arbitrary order), so the
> > > > marked_independent flag does carry non-redundant information --
> > > > even with safelen set to INT_MAX.  
> > > 
> > > OpenMP 5 (not implemented in GCC 9 though) has order(concurrent)
> > > clause for this (no cross-iteration dependencies at all, iterations
> > > can be run in any order, in parallel etc.).
> > > 
> > > I believe it matches the can_be_parallel flag we now have, but I
> > > remember there were some issues with that flag for use in DO
> > > CONCURRENT.
> > > 
> > > Or do we want to have some other flag for really independent
> > > iterations? What passes could use that?  Would the vectorizer
> > > appreciate the stronger assertion in some cases?  
> > 
> > The vectorizer doesn't really care.  It would be autopar that should.
> > The issue with using can_be_parallel for DO CONCURRENT was that the
> > middle-end introduces non-trivial sharing between iterations,
> > introducing dependences that then make the loop no longer
> > can_be_parallel.  I believe similar things could happen with
> > ->safelen (consider loop reversal and existing forward dependences).
> > I guess we're simply lucky in that area ;)
> 
> I wondered if I should try modifying the patch to set the
> can_be_parallel flag for kernels loops with an "independent" clause
> instead (and try to address Jakub's other comments). Do I understand
> right that the issue with the can_be_parallel flag is that it does not
> necessarily guarantee safety of optimisations for loops which are
> supposed to have fully-independent iterations, rather than that it has
> different semantics from the proposed marked_independent flag?
> 
> However, it turns out that this patch has a dependency on this one:
> 
>   https://gcc.gnu.org/ml/gcc-patches/2018-09/msg01179.html
> 
> and, according to Cesar, that in turn has a dependency on another patch:
> 
>   https://gcc.gnu.org/ml/gcc-patches/2018-09/msg01189.html
> 
> so, it might take me a little time to untangle all that. Does the rough
> idea sound plausible, though? Or is modifying this patch to use
> can_be_parallel likely to just cause problems at present?

For Fortran DO CONCURRENT it caused problems.  can_be_parallel causes
auto-parallelization to omit all dependence checking which works
fine for the small time this flag is usually set by the graphite
dependence analysis (which marks loops that way) and autopar itself
(which is also the only consumer of the flag right now).  But if
the frontend already sets the flag there's too much chance for the
middle-end to mess up dependences and thus the autopar code would
need to double-check (also making the flag somewhat useless).

Richard.

> Thanks,
> 
> Julian
> 
> 

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

Re: [PATCH] Improve predictions for hot and cold labels ([[likely]], [[unlikely]]).

[Bernhard Reutner-Fischer]

On 30 November 2018 10:47:45 CET, "Martin Liška"  wrote:
>Hi.
>
>This patch is a reaction to Jason's commit where he introduced new C++
>attributes.
>First I would like to align cold/hot to __builtin_expect, so I adjusted
>probability
>and made the predictors first match predictors.
>
>Second I fixed how we consider the predictors in switch statements, so
>that
>we can correctly predict situation in predict-3.
>
>Honza is fine with the patch, I'll install it later if there are no
>objections.
>Survives tests and bootstrap on xc86_64-linux-gnu.

I don't have the sources at hand but in:

+/* Branches to hot labels are likely.  */
+DEF_PREDICTOR (PRED_HOT_LABEL, "hot label", HITRATE (90),
+  PRED_FLAG_FIRST_MATCH)
+
+/* Branches to cold labels are extremely unlikely.  */
+DEF_PREDICTOR (PRED_COLD_LABEL, "cold label", HITRATE (90),
+  PRED_FLAG_FIRST_MATCH)
+

I would have expected cold labels to have a rather low hitrate, like maybe 2 or 
7, not 90 ?

Thanks,