Re: [PATCH] Unconditionally use MAX_EXPR/MIN_EXPR for MAX/MIN intrinsics

[Janne Blomqvist]

On Sun, Aug 19, 2018 at 10:47 PM Thomas Koenig 
wrote:

> Hi Janne,
>
> > On Fri, Aug 10, 2018 at 11:47 PM, Janne Blomqvist <
> blomqvist.ja...@gmail.com
> >> wrote:
> >
> >> For floating point types, the question is what MAX(a, NaN) or MIN(a,
> >> NaN) should return (where "a" is a normal number).  There are valid
> >> usecases for returning either one, but the Fortran standard doesn't
> >> specify which one should be chosen.  Also, there is no consensus among
> >> other tested compilers.  In short, it's a mess.  So lets just do
> >> whatever is fastest, which is using MAX_EXPR/MIN_EXPR which are not
> >> defined to do anything in particular if one of the operands is a NaN.
> >>
> >> Regtested on x86_64-pc-linux-gnu, Ok for trunk?
>
> OK.
>

Thanks, committed.


>
> Could you also document this behavior in the "Compiler Characteristics"
> section, and make mention of the change in gcc-9/changes.html ?
>

Yes, done. I also updated the News page in the wiki.

-- 
Janne Blomqvist

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

[Tom de Vries]

[ was: Re: [RFC][debug] Add -greadable-dwarf ]

On 08/21/2018 02:22 PM, Tom de Vries wrote:
> Currently doing a bootstrap and reg-test on x86_64 of attached patch
> with -gforce-named-dies enabled by default.

That went well. There are some understandable failures in
libstdc++-prettyprinters/whatis.cc, explained in the rationale below.

OK for trunk?

Thanks,
- Tom
[debug] Add -gforce-named-dies

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

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

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

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

This is an undocumented developer-only option, because using this option may
change behaviour of dwarf consumers, f.i., gdb shows the artificial variables:
...
(gdb) info locals
a = 0x7fffda90 "\005"
D.4278 = 
...

Bootstrapped and reg-tested on x86_64, in combination with a patch that
switches the option on by default.  The only failures are in
libstdc++-prettyprinters/whatis.cc, where we get failures of the following
type: we expect to gdb to print
...
type = holder
...
but instead we get
...
  type = holder > >
...
where std::ios is defined as:
...
 typedef basic_ios   ios;
 template > class basic_ios;
...

2018-08-15  Tom de Vries  

	* common.opt (gforce-named-dies): Add option.
	* dwarf2out.c (add_name_and_src_coords_attributes): Add param. Add name
	for artifical and nameless decls.
	(dwarf2out_register_external_die): Add name to external reference die.
	(gen_subprogram_die): Add name to DW_TAG_call_site_parameter.

---
 gcc/common.opt  |  5 +
 gcc/dwarf2out.c | 27 +++
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/gcc/common.opt b/gcc/common.opt
index ebc3ef43ce2..76032f9bb1d 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2976,6 +2976,11 @@ gstrict-dwarf
 Common Driver Report Var(dwarf_strict) Init(0)
 Don't emit DWARF additions beyond selected version.
 
+gforce-named-dies
+Common Driver Undocumented Report Var(flag_force_named_dies) Init(0)
+Force DWARF DIEs to have a name attribute.  Undocumented because it exposes
+compiler internals to the DWARF consumer.
+
 gtoggle
 Common Driver Report Var(flag_gtoggle)
 Toggle debug information generation.
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index fb71ff349fa..dd8f438dfd3 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -3824,7 +3824,8 @@ static void add_prototyped_attribute (dw_die_ref, tree);
 static dw_die_ref add_abstract_origin_attribute (dw_die_ref, tree);
 static void add_pure_or_virtual_attribute (dw_die_ref, tree);
 static void add_src_coords_attributes (dw_die_ref, tree);
-static void add_name_and_src_coords_attributes (dw_die_ref, tree, bool = false);
+static void add_name_and_src_coords_attributes (dw_die_ref, tree, bool = false,
+		bool = false);
 static void add_discr_value (dw_die_ref, dw_discr_value *);
 static void add_discr_list (dw_die_ref, dw_discr_list_ref);
 static inline dw_discr_list_ref AT_discr_list (dw_attr_node *);
@@ -6022,6 +6023,8 @@ dwarf2out_register_external_die (tree decl, const char *sym,
   else
 equate_decl_number_to_die (decl, die);
 
+  if (flag_force_named_dies && DECL_P (decl))
+add_name_and_src_coords_attributes (die, decl, true, true);
   /* Add a reference to the DIE providing early debug at $sym + off.  */
   add_AT_external_die_ref (die, DW_AT_abstract_origin, sym, off);
 }
@@ -21277,22 +21280,33 @@ add_linkage_name (dw_die_ref die, tree decl)
 
 static void
 add_name_and_src_coords_attributes (dw_die_ref die, tree decl,
-bool no_linkage_name)
+bool no_linkage_name,
+bool no_src_coords_attributes)
 {
   tree decl_name;
 
   decl_name = DECL_NAME (decl);
   if (decl_name != NULL && IDENTIFIER_POINTER (decl_name) != NULL)
 {
-  const char *name = dwarf2_name (decl, 0);
+  const char *name = (flag_force_named_dies && DECL_NAMELESS (decl)
+			  ? IDENTIFIER_POINTER (decl_name)
+			  : dwarf2_name (decl, 0));
   if (name)
 	add_name_attribute (die, name);
-  if (! DECL_ARTIFICIAL (decl))
+  if (! no_src_coords_attributes && ! DECL_ARTIFICIAL (decl))
 	add_src_coords_attributes (die, decl);
 
   if (!no_linkage_name)
 	add_linkage_name (die, decl);
 }
+  else if (flag_force_named_dies && decl_name == NULL
+	   && (TREE_CODE (decl) == VAR_DECL || TREE_CODE (decl) == CONST_DECL))
+{
+  char buf[32];
+  char decl_letter = TREE_CODE (decl) == CONST_DECL ? 'C' : 'D';
+  sprintf (buf, "%c.%u", decl_lett

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

[Uecker, Martin]

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

There is a target hook

TARGET_CUSTOM_FUNCTION_DESCRIPTORS

But I am not sure how to get this information to the testsuite.

> For tests for features supported on some but not all targets, we use 
> effective-target keywords.  Of course you need to be careful about how you 
> implement those keywords: you don't want the implementation of the keyword 
> to be essentially the same as the test being conditioned, to avoid a bug 
> in the implementation quietly causing the test to be disabled.  But the 
> implementation of the keyword might e.g. have a blacklist of targets that 
> do not yet support the feature, with the expectation that the test should 
> run and pass on all other targets.

gcc/testsuite/lib/target-supports.exp

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

Best,
Martin

Re: [PATCH] Make GO string literals properly NUL terminated

[Bernd Edlinger]

On 08/21/18 10:33, Richard Biener wrote:
> On Mon, 20 Aug 2018, Bernd Edlinger wrote:
> 
>> On 08/20/18 15:19, Richard Biener wrote:
>>> On Mon, 20 Aug 2018, Bernd Edlinger wrote:
>>>
 On 08/20/18 13:01, Richard Biener wrote:
> On Wed, Aug 1, 2018 at 3:05 PM Bernd Edlinger  
> wrote:
>>
>>
>>
>> On 08/01/18 11:29, Richard Biener wrote:
>>>
>>> Hmm.  I think it would be nice if TREE_STRING_LENGTH would
>>> match char[2] and TYPE_SIZE_UNIT even if that is inconvenient
>>> for your check above.  Because the '\0' doesn't belong to the
>>> string.  Then build_string internally appends a '\0' outside
>>> of TREE_STRING_LENGTH.
>>>
>>
>> Hmm. Yes, but the outside-0 byte is just one byte, not a wide
>> character.
>
> That could be fixed though (a wide 0 is just N 0s).  Add a elsz = 1
> parameter to build_string and allocate as many extra 0s as needed.
>
>  There are STRING_CSTs which are not string literals,
>> for instance attribute tags, Pragmas, asm constrants, etc.
>> They use the '\0' outside, and have probably no TREE_TYPE.
>>
>>>
 So I would like to be able to assume that the STRING_CST objects
 are internally always generated properly by the front end.
>>>
>>> Yeah, I guess we need to define what "properly" is ;)
>>>
>> Yes.
>>
 And that the ARRAY_TYPE of the string literal either has the
 same length than the TREE_STRING_LENGTH or if it is shorter,
 this is always exactly one (wide) character size less than 
 TREE_STRING_LENGTH
>>>
>>> I think it should be always the same...
>>>
>>
>> One could not differentiate between "\0" without zero-termination
>> and "" with zero-termination, theoretically.
>
> Is that important?  Doesn't the C standard say how to parse string 
> literals?
>
>> We also have char x[100] = "ab";
>> that is TREE_STRING_LENGTH=3, and TYPE_SIZE_UNIT(TREE_TYPE(x)) = 100.
>> Of course one could create it with a TREE_STRING_LENGTH = 100,
>> but imagine char x[1000] = "ab"
>
> The question is more about TYPE_SIZE_UNIT (TREE_TYPE ("ab")) which I
> hope matches "ab" and not 'x'.  If it matches 'x' then I'd rather have it
> unconditionally be [], thus incomplete (because the literals "size" 
> depends
> on the context/LHS it is used on).
>

 Sorry, but I must say, it is not at all like that.

 If I compile x.c:
 const char x[100] = "ab";

 and set a breakpoint at output_constant:

 Breakpoint 1, output_constant (exp=0x76ff9dc8, size=100, align=256,
reverse=false) at ../../gcc-trunk/gcc/varasm.c:4821
 4821 if (size == 0 || flag_syntax_only)
 (gdb) p size
 $1 = 100
 (gdb) call debug(exp)
 "ab"
 (gdb) p *exp
 $2 = {base = {code = STRING_CST, side_effects_flag = 0, constant_flag = 1,
 (gdb) p exp->typed.type->type_common.size_unit
 $5 = (tree) 0x76ff9d80
 (gdb) call debug(exp->typed.type->type_common.size_unit)
 100
 (gdb) p exp->string.length
 $6 = 3
 (gdb) p exp->string.str[0]
 $8 = 97 'a'
 (gdb) p exp->string.str[1]
 $9 = 98 'b'
 (gdb) p exp->string.str[2]
 $10 = 0 '\000'
 (gdb) p exp->string.str[3]
 $11 = 0 '\000'


 This is an important property of string_cst objects, that is used in 
 c_strlen:

 It folds c_strlen(&x[4]) directly to 0, because every byte beyond 
 TREE_STRING_LENGTH
 is guaranteed to be zero up to the type size.

 I would not have spent one thought on implementing an optimization like 
 that,
 but that's how it is right now.
>>>
>>> Huh.  So somebody interpreted STRING_CSTs similar to CONSTRUCTORs aka
>>> they have zero-padding up to its type size.  I don't see this documented
>>> anywhere and it would suggest to "optimize" "ab\0\0\0\0" to "ab\0"
>>> with appropriate TYPE_SIZE.
>>>
>>> This is also a relatively new thing on trunk (coming out of the added
>>> mem_size parameter of string_constant).  That this looks at the STRING_CST
>>> type like
>>>
>>> if (TREE_CODE (array) == STRING_CST)
>>>   {
>>> *ptr_offset = fold_convert (sizetype, offset);
>>> if (mem_size)
>>>   *mem_size = TYPE_SIZE_UNIT (TREE_TYPE (array));
>>> return array;
>>>
>>> I'd call simply a bug.  As said, interpretation of STRING_CSTs should
>>> depend on the context.  And for
>>>
>>
>> This use of the TYPE_SIZE_UNIT was pre-existing to r263607
>> before that (but not in the gcc-8-branch) we had this in c_strlen:
>>
>> HOST_WIDE_INT maxelts = strelts;
>> tree type = TREE_TYPE (src);
>> if (tree size = TYPE_SIZE_UNIT (type))
>>   if (tree_fits_shwi_p (size))
>> {
>>  maxelts = tree_to_uhwi (size);
>>  maxelts = maxelts / eltsize - 1;
>> }
>>
>> which I moved to stri

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

[Bernd Edlinger]

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

and in this case:

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

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


string_constant works out the initializer for u.x
which has a different type than u.z


> Richard.
> 
>>
>> Bernd.
>>
> I would just like the ability to get the length/size somehow
> so that the questionable code that started us down this path
> can be detected.  This is not just the sprintf(d, "%s", L"1")
> kind of a mismatch but also the missing nul detection in cases
> like:
 [ ... ]
 I know.  And ideally we'll be able to handle everything you want to.
 But we also have to realize that sometimes we may have to punt.

 Also remember that incremental progress is (usually) good.


>
>    const wchar_t a[1] = L"\x";
>    strcpy(d, (char*)a);
 This touches on both the representational issue (excess elements in the
 initializer) and how to handle a non-terminated string.  Both are issues
 we're debating right now.

>
> I don't think this is necessarily an important use case to
> optimize for but it is one that GCC optimizes already nonetheless,
> and always has.  For example, this has always been folded into 4
> by GCC and still is even with the patch:
>
>    const __WCHAR_TYPE__ wc[] = L"\x12345678";
>
>    int f (void)
>    {
>      const char *p = (char*)wc;
>      return strlen (p);    // folded to 4
>    }
>
> It's optimized because fold_const_call() relies on c_getstr()
> which returns the underlying byte representation of the wide
> string, and despite c_strlen() now trying to prevent it.
 And I think you're hitting on some of issues already raised in the thread.

 In this specific case though ISTM 4 is the right answer.  We casted to a
 char *, so that's what we should be counting.  Or am I missing
 something?  Also note that's the value we'd get from the strlen C
 library call IIUC.
>>>
>>> It is the right answer.  My point is that this is optimized
>>> but the change to c_strlen() prevents the same optimization
>>> in other similar cases.  For example, GCC 6 folds this into
>>> memcpy:
>>>
>>>     __builtin_strcpy (d, (char*)L"\x12345678");
>>>
>>> GCC 7 and 8 do too but get the byte count wrong (my bad).
>>>
>>> Current trunk doesn't optimize it.  If restoring the original
>>> behavior is the intent (and not just fixing the counting bug)
>>> then c_strlen() should be fixed to fold this again.
>>>
>>> (I'm not a fan of the strcpy to memcpy transformation because
>>> it makes other things difficult but that's beside the point.)
>>>
> The missing nul variant of the same test case isn't folded (it
> ends up calling the library strlen) but the bug cannot be
> detected using the modified c_strlen():
>
>    const __WCHAR_TYPE__ wc[1] = L"\x12345678";   // no nul
>
>    int f (void)
>    {
>      const char *p = (char*)wc;
>      return strlen (p);    // not folded
>    }
>
> To detect it, I'd have to use some other function, like
> c_getstr().  I could certainly do that but I don't think
> I should need to.
 And that's on the list of things we're going to try and address next.  I
 don't think it needs to be conflated with change which indicated to
 c_strlen how to count.
>>>
>>> The two are one and the same: if c_strlen() doesn't count bytes
>>> in arrays of wide characters (wchar_t, char16_t, or char32_t)
>>> then the original GCC behavior is not restored and the missing
>>> nul detection won't work for these "mixed type" cases.
>>>
 I do hope you're getting all these testcases recorded.  I'd even support
 installing them into the suite now, properly xfailed.  It's easier to
 see how any patch under consideration affects the wider set of tests.
>>>
>>> I haven't been recording any of them --  I have no idea where
>>> this is going.  As I've said, these patch

Re: [PATCH] Make strlen range computations more conservative

[Bernd Edlinger]

On 08/22/18 00:43, Jeff Law wrote:
> [ I'm still digesting, but saw something in this that ought to be broken
> out... ]
> 
> On 08/19/2018 09:55 AM, Bernd Edlinger wrote:
>> diff -Npur gcc/tree-ssa-dse.c gcc/tree-ssa-dse.c
>> --- gcc/tree-ssa-dse.c   2018-07-18 21:21:34.0 +0200
>> +++ gcc/tree-ssa-dse.c   2018-08-19 14:29:32.344498771 +0200
>> @@ -248,6 +248,12 @@ compute_trims (ao_ref *ref, sbitmap live
>>   residual handling in mem* and str* functions is usually
>>   reasonably efficient.  */
>> *trim_tail = last_orig - last_live;
>> +  /* Don't fold away an out of bounds access, as this defeats proper
>> + warnings.  */
>> +  if (*trim_tail
>> +  && compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (ref->base)),
>> +   last_orig) <= 0)
>> +*trim_tail = 0;
>>   }
>> else
>>   *trim_tail = 0;
> This seems like a good change in and of itself and should be able to go
> forward without further review work.   Consider this hunk approved,
> along with any testsuite you have which tickles this code (I didn't
> immediately see one attached to this patch.  But I could have missed it).
> 

Sorry, for not being clear on this.

I needed both hunks "Don't fold away an out of bounds access, as this defeats 
proper
warnings" to prevent a regression on gcc.dg/Wstringop-overflow-5.c,
and surprise surprise, the xfail in gcc.dg/Wstringop-overflow-6.c suddenly 
popped up.

So without the unsafe range info, gcc.dg/Wstringop-overflow-5.c needs both hunks
to not regress, but gcc.dg/Wstringop-overflow-6.c only needs the other one I 
committed
yesterday.

So unfortunately I have no test case except gcc.dg/Wstringop-overflow-5.c for 
that.


Still OK?


Bernd.

Re: [PATCH] Make strlen range computations more conservative

[Bernd Edlinger]

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

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

The test cases are:
gcc.dg/strlenopt-36.c
gcc.dg/strlenopt-40.c
gcc.dg/strlenopt-45.c
gcc.dg/strlenopt-48.c
gcc.dg/strlenopt-51.c

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

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

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

   size_t len = __builtin_strlen (src);
   if (len < size)
 __builtin_memcpy (dst, src, len + 1);
   else
 {
   __builtin_memcpy (dst, src, size - 1); /* { dg-bogus 
"\\\[-Wstringop-oveflow]" } */
   dst[size - 1] = '\0';
 }

I have not fully debugged that, but believe the test case works because unsafe
range infos are used to eliminate the memcpy call before it is diagnosed.


> I think we can all agree that if a test depends on C semantics rather
> than GIMPLE semantics for optimization/codegen, then removing it seems
> like the right thing to do as the test is just wrong for GCC.
> 
> If the test is meant to issue a warning or avoid a false positive, then
> we should defer -- I'd like to not lose the warnings if at all possible.
> 

yes, this patch is looking pretty healthy right now.
except the one regression above, there are no further regressions on
the warnings.  Just the code generation does change.

There are however still more correctness issues:
There is still an information flow from
get_range_strlen -> sprintf return code -> VRP.

And generally, even if using the range info from the sprintf function
is fine from the posix spec, I still have a bad feeling about that,
because "sprintf" is a rather complex piece of software that can even
have bugs or implementation details, I think of non-posix environments
like linux or ecos which come with an own implementation of sprintf
and those may have subtle differences, or simply bugs, but this
optimization takes those the right away to return an error code.


> A test which verifies correct optimization seems like it should be
> discussed.  I'd be more inclined to xfail those rather than remove them
> completely -- particularly for a test which isn't a good indicator of
> the real world code typically seen by gcc.
> 
xfail would work for me, but I doubt we will ever be able to fix a test case
that does so many different things at the same time, and checks just if
all was folded or nothing.


Bernd.

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

[Martin Sebor]

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

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

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

Martin

PR middle-end/87052 - STRING_CST printing incomplete in Gimple dumps

gcc/testsuite/ChangeLog:

	PR middle-end/87052
	* gcc.dg/pr87052.c: New test.

gcc/ChangeLog:

	PR middle-end/87052
	* tree-pretty-print.c (pretty_print_string): Add argument.
	(dump_generic_node): Call to pretty_print_string with string size.

Index: gcc/tree-pretty-print.c
===
--- gcc/tree-pretty-print.c	(revision 263754)
+++ gcc/tree-pretty-print.c	(working copy)
@@ -37,7 +37,7 @@ along with GCC; see the file COPYING3.  If not see
 
 /* Local functions, macros and variables.  */
 static const char *op_symbol (const_tree);
-static void pretty_print_string (pretty_printer *, const char*);
+static void pretty_print_string (pretty_printer *, const char*, unsigned);
 static void newline_and_indent (pretty_printer *, int);
 static void maybe_init_pretty_print (FILE *);
 static void print_struct_decl (pretty_printer *, const_tree, int, dump_flags_t);
@@ -1800,10 +1800,13 @@ dump_generic_node (pretty_printer *pp, tree node,
   break;
 
 case STRING_CST:
-  pp_string (pp, "\"");
-  pretty_print_string (pp, TREE_STRING_POINTER (node));
-  pp_string (pp, "\"");
-  break;
+  {
+	pp_string (pp, "\"");
+	if (unsigned nbytes = TREE_STRING_LENGTH (node))
+	  pretty_print_string (pp, TREE_STRING_POINTER (node), nbytes);
+	pp_string (pp, "\"");
+	break;
+  }
 
 case VECTOR_CST:
   {
@@ -3865,15 +3868,16 @@ print_call_name (pretty_printer *pp, tree node, du
 }
 }
 
-/* Parses the string STR and replaces new-lines by '\n', tabs by '\t', ...  */
+/* Print the first N characters in the array STR, replacing non-printable
+   characters (including embedded nuls) with unambiguous escape sequences.  */
 
 static void
-pretty_print_string (pretty_printer *pp, const char *str)
+pretty_print_string (pretty_printer *pp, const char *str, unsigned n)
 {
   if (str == NULL)
 return;
 
-  while (*str)
+  for ( ; n; --n, ++str)
 {
   switch (str[0])
 	{
@@ -3913,48 +3917,20 @@ static void
 	  pp_string (pp, "\\'");
 	  break;
 
-	  /* No need to handle \0; the loop terminates on \0.  */
-
-	case '\1':
-	  pp_string (pp, "\\1");
-	  break;
-
-	case '\2':
-	  pp_string (pp, "\\2");
-	  break;
-
-	case '\3':
-	  pp_string (pp, "\\3");
-	  break;
-
-	case '\4':
-	  pp_string (pp, "\\4");
-	  break;
-
-	case '\5':
-	  pp_string (pp, "\\5");
-	  break;
-
-	case '\6':
-	  pp_string (pp, "\\6");
-	  break;
-
-	case '\7':
-	  pp_string (pp, "\\7");
-	  break;
-
 	default:
-	  if (!ISPRINT (str[0]))
+	  if (str[0] || n > 1)
 	{
-	  char buf[5];
-	  sprintf (buf, "\\x%x", (unsigned char)str[0]);
-	  pp_string (pp, buf);
+	  if (!ISPRINT (str[0]))
+		{
+		  char buf[5];
+		  sprintf (buf, "\\x%02x", (unsigned char)str[0]);
+		  pp_string (pp, buf);
+		}
+	  else
+		pp_character (pp, str[0]);
+	  break;
 	}
-	  else
-	pp_character (pp, str[0]);
-	  break;
 	}
-  str++;
 }
 }
 
Index: gcc/testsuite/gcc.dg/pr87052.c
===
--- gcc/testsuite/gcc.dg/pr87052.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/pr87052.c	(working copy)
@@ -0,0 +1,41 @@
+/* PR middle-end/87052 - STRING_CST printing incomplete in Gimple dumps
+   { dg-do compile }
+   { dg-options "-fdump-tree-gimple" } */
+
+void sink (const void*, ...);
+
+void test (void)
+{
+  const char a[3] = "\000ab";
+
+  /* Expect the following in the dump:
+ a = "\x00ab"; */
+
+  const char b[] = { 'a', 0, 'b', 'c' };
+
+  /* Expect the following:
+ b = "a\x00bc"; */
+
+  const char c[] = "";
+
+  /* Expect the following:
+ c = ""; */
+
+  const char d[0] = { };
+
+  /* Expect the following:
+ d = ""; */
+
+  const char e[0] = "";
+
+  /* Expect nothing.  */
+
+  sink (a, b, c, d, e);
+}
+
+/* { dg-final { scan-tree-dump-times "a = \"x00ab\";" 1 "gimple" } }
+   { dg-final { scan-tree-dump-times "b = \"ax00bc\";"  1 "gimple" } }
+   { dg-final { scan-tree-dump-times "c = \"\";"  1 "gimple" } }
+   { dg-final { scan-tree-dump-times "d = { *};"  1 "gimple" } }
+   { dg-final { scan-tree-dump-times "e = "

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

[Martin Sebor]

On 08/21/2018 03:44 PM, Joseph Myers wrote:

On Tue, 21 Aug 2018, Martin Sebor wrote:


On 08/21/2018 09:44 AM, Joseph Myers wrote:

On Tue, 21 Aug 2018, Martin Sebor wrote:


Sure, but the only valid argument to %ls is wchar_t*.  Passing
it something else is undefined.


Well, (wchar_t *)"something\0\0\0\0" would be OK given
-fno-strict-aliasing and if you know the alignment is OK.  Do we have that
information about the type cast to, as opposed to the type of the string
constant, at this point?


In the simple cases like the one above the cast is gone.  Only
in some more involved cases is the type of the argument preserved.
I responded to Jeff with one such example here:

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

If supporting (wchar_t *)"...\0\0\0\0" with %ls is viewed as
important (despite it being undefined) then the function does


There are different cases of support.  It doesn't need to be highly
optimized or get particularly good diagnostics.  It does need to avoid
being miscompiled or getting actively incorrect diagnostics.


Sure.


Given -fno-strict-aliasing and appropriate alignment, it's not undefined.
(To ensure appropriate alignment one might use a target where
BIGGEST_ALIGNMENT is 8, or one where it is 16 and a char16_t[] string
constant is cast to pointer to 32-bit wchar_t.)


I was referring to the undefined behavior according to the letter
of the C standard which says about %ls:

  If an l length modifier is present, the argument shall be
  a pointer to the initial element of an array of wchar_t type.

Despite the cast, (wchar_t*)"...\0\0\0" is an array of char,
not one of wchar_t type.  The same would be true if the cast
were to void* or any other object pointer type other that
wchar_t.

But now we're in a language lawyer territory and I'm pretty
sure you were making a point about the library call having
the intuitive semantics of interpreting the narrow string
representation as an array of wchar_t.

In any case, I have posted a prototype to issue a warning for
these mismatches as has been suggested that should have these
same semantics, or could easily be tweaked to achieve them if
or where it doesn't.  To goal is to show the approach I had
in mind to make good diagnostics possible.  I'm open to
fine-tuning the details.

Martin

Re: Richard Sandiford appointed Global Reviewer

[Bin.Cheng]

On Tue, Aug 21, 2018 at 9:59 PM Richard Sandiford
 wrote:
>
> David Edelsohn  writes:
> >   I am pleased to announce that the GCC Steering Committee has
> > appointed Richard Sandiford as a Global Reviewer.
> >
> >   Please join me in congratulating Richard on his new role.
> > Richard, please update your listing in the MAINTAINERS file.
>
> Thanks David, and thanks SC!  I've installed the patch below.

Great, and Congratulations!

Thanks,
bin
>
> Richard
>
>
> 2018-08-21  Richard Sandiford  
>
> * MAINTAINERS: Add self to global reviewers list.
>
> Index: MAINTAINERS
> ===
> --- MAINTAINERS 2018-08-21 14:47:08.771159612 +0100
> +++ MAINTAINERS 2018-08-21 14:47:22.635042692 +0100
> @@ -29,6 +29,7 @@ Michael Meissner  
>   Jason Merrill  
>  David S. Miller
>  Joseph Myers   
> +Richard Sandiford  
>  Bernd Schmidt  
>  Ian Lance Taylor   
>  Jim Wilson 

[PATCH] warn for sprintf argument mismatches (PR 87034)

[Martin Sebor]

It didn't seem like we were making progress in the debate about
warning for sprintf argument mismatches earlier today so I took
a couple of hours this afternoon to prototype one of the solutions
I was trying to describe.  It mostly keeps the existing interface
and just extends c_strlen() and the other functions to pass in
an in-out argument describing the requested element size on input
and the constant string on output.  The caller is responsible for
validating the string to make sure its type matches the expected
type.

String functions like strcpy interested in the size of their
argument in bytes succeed even for a wide string argument and
are candidates for folding (this matches the original behavior).
The patch doesn't add any warning for mismatched calls to those
(such as strcpy(d, (char*)L"123");) but enhancing it to do that
would be just as "simple" as adding the missing nul detection.

Calls to sprintf "%s" and "%ls" also succeed with mismatched
arguments but get a warning:

warning: ‘%s’ invalid directive argument type ‘int[4]’ [-Wformat-overflow=]
3 |   __builtin_sprintf (d, "%s", (char*)L"123");
  |  ^~  ~~
warning: ‘%ls’ invalid directive argument type ‘char[4]’ 
[-Wformat-overflow=]

4 |   __builtin_sprintf (d, "%ls", (__WCHAR_TYPE__*)"123");
  |  ^~~~

FWIW, I chose the approach of adding a c_strlen_data structure
over adding yet another argument to c_strlen() and friends to
keep the argument list from getting too long and confusing
(like get_range_strlen).  This should also make it easy to
retrofit the missig nul detection patch on top of it.

I didn't take any time to add tests for the restored strcpy
(et al.) folding.

If this looks like the general direction we can agree on (perhaps
with some tweaks, including not folding etc.) I will add those
tests, plus more for the various argument mismatches.

Tested on x86_64-linux.

Martin
PR tree-optimization/87034 - missing -Wformat-overflow on a sprintf %s with a wide string

gcc/ChangeLog:

	PR tree-optimization/87034
	* builtins.c (c_strlen): Add an argument.  Optionally return string
	to caller.
	* builtins.h (c_strlen): Add an argument.
	* gimple-fold.c (get_range_strlen): Replace argument with
	c_strlen_data *.
	(get_maxval_strlen): Adjust call to get_range_strlen.
	(gimple_fold_builtin_strlen): Same.
	* gimple-fold.h (c_strlen_data): New struct.
	(get_range_strlen): Add optional argument.
	* gimple-ssa-sprintf.c (get_string_length): Change argument type.
	(format_string): Same.  Adjust.
	(format_directive): Diagnose incompatible arguments.

gcc/testsuite/ChangeLog:

	PR tree-optimization/87034
	* gcc.dg/tree-ssa/builtin-sprintf-warn-20.c: Remove xfail.

Index: gcc/builtins.c
===
--- gcc/builtins.c	(revision 263754)
+++ gcc/builtins.c	(working copy)
@@ -568,15 +568,12 @@ string_length (const void *ptr, unsigned eltsize,
accesses.  Note that this implies the result is not going to be emitted
into the instruction stream.
 
-   ELTSIZE is 1 for normal single byte character strings, and 2 or
-   4 for wide characer strings.  ELTSIZE is by default 1.
 
The value returned is of type `ssizetype'.  */
 
 tree
-c_strlen (tree src, int only_value, unsigned eltsize)
+c_strlen (tree src, int only_value, c_strlen_data *pdata /* = NULL */)
 {
-  gcc_assert (eltsize == 1 || eltsize == 2 || eltsize == 4);
   STRIP_NOPS (src);
   if (TREE_CODE (src) == COND_EXPR
   && (only_value || !TREE_SIDE_EFFECTS (TREE_OPERAND (src, 0
@@ -583,8 +580,8 @@ tree
 {
   tree len1, len2;
 
-  len1 = c_strlen (TREE_OPERAND (src, 1), only_value, eltsize);
-  len2 = c_strlen (TREE_OPERAND (src, 2), only_value, eltsize);
+  len1 = c_strlen (TREE_OPERAND (src, 1), only_value, pdata);
+  len2 = c_strlen (TREE_OPERAND (src, 2), only_value, pdata);
   if (tree_int_cst_equal (len1, len2))
 	return len1;
 }
@@ -591,7 +588,7 @@ tree
 
   if (TREE_CODE (src) == COMPOUND_EXPR
   && (only_value || !TREE_SIDE_EFFECTS (TREE_OPERAND (src, 0
-return c_strlen (TREE_OPERAND (src, 1), only_value, eltsize);
+return c_strlen (TREE_OPERAND (src, 1), only_value, pdata);
 
   location_t loc = EXPR_LOC_OR_LOC (src, input_location);
 
@@ -602,10 +599,16 @@ tree
   if (src == 0)
 return NULL_TREE;
 
-  /* Determine the size of the string element.  */
-  if (eltsize != tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (src)
-return NULL_TREE;
+  unsigned eltsize = 1;
+  if (pdata)
+{
+  eltsize = pdata->eltsize;
 
+  tree srctype = TREE_TYPE (TREE_TYPE (src));
+  if (eltsize != tree_to_uhwi (TYPE_SIZE_UNIT (srctype)))
+	pdata->string = src;
+}
+
   /* Set MAXELTS to sizeof (SRC) / sizeof (*SRC) - 1, the maximum possible
  length of SRC.  Prefer TYPE_SIZE() to TREE_STRING_LENGTH() if possible
  in case the latter is less than the size of the array, su

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

[Paul Hua]

On Wed, Aug 22, 2018 at 2:15 AM Qing Zhao  wrote:
>
>
> > On Aug 21, 2018, at 8:07 AM, Paul Hua  wrote:
> >
> > Hi, Qing,
> >
> > The cfarm machine gcc23 can build mips64el successful, configure with
> > "../configure --prefix=/opt/gcc-9 MISSING=texinfo MAKEINFO=missing
> > --target=mips64el-linux-gnu --enable-languages=c,c++
>
> I got the same failure message on gcc23 as gcc22, even with the above 
> configure:
>
> /usr/bin/ld: failed to merge target specific data of file 
> /usr/lib32/libc.a(mremap.o)
> /usr/bin/ld: /usr/lib32/libc.a(libc-lowlevellock.o): ABI is incompatible with 
> that of the selected emulation
>
> not sure what’s the problem?
>

I just build all-gcc and make check.

./configure xxx
make all-gcc -j 2
make check-gcc RUNTESTFLAGS="dg.exp=strcmpopt_6.c"

It's enough to reproduce the regression.

Here is a mips port patch.

diff --git a/gcc/testsuite/gcc.dg/strcmpopt_6.c
b/gcc/testsuite/gcc.dg/strcmpopt_6.c
index 4c6de02824f..fa0ff9d1171 100644
--- a/gcc/testsuite/gcc.dg/strcmpopt_6.c
+++ b/gcc/testsuite/gcc.dg/strcmpopt_6.c
@@ -33,4 +33,5 @@ int main (void)

 }

-/* { dg-final { scan-assembler-times "memcmp" 2 } } */
+/* { dg-final { scan-assembler-times "memcmp" 2 { target { !
mips*-*-* } } } } */
+/* { dg-final { scan-assembler-times "memcmp" 4 { target { mips*-*-* } } } } */

Paul Hua

Re: [PATCH] Make strlen range computations more conservative

[Jeff Law]

[ I'm still digesting, but saw something in this that ought to be broken
out... ]

On 08/19/2018 09:55 AM, Bernd Edlinger wrote:
> diff -Npur gcc/tree-ssa-dse.c gcc/tree-ssa-dse.c
> --- gcc/tree-ssa-dse.c2018-07-18 21:21:34.0 +0200
> +++ gcc/tree-ssa-dse.c2018-08-19 14:29:32.344498771 +0200
> @@ -248,6 +248,12 @@ compute_trims (ao_ref *ref, sbitmap live
>residual handling in mem* and str* functions is usually
>reasonably efficient.  */
>*trim_tail = last_orig - last_live;
> +  /* Don't fold away an out of bounds access, as this defeats proper
> +  warnings.  */
> +  if (*trim_tail
> +   && compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (ref->base)),
> +last_orig) <= 0)
> + *trim_tail = 0;
>  }
>else
>  *trim_tail = 0;
This seems like a good change in and of itself and should be able to go
forward without further review work.   Consider this hunk approved,
along with any testsuite you have which tickles this code (I didn't
immediately see one attached to this patch.  But I could have missed it).

Jeff

Re: [PATCH] Make strlen range computations more conservative

[Jeff Law]

On 08/21/2018 02:43 AM, Richard Biener wrote:
> On Mon, 20 Aug 2018, Bernd Edlinger wrote:
[ snip. ]

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

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

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

I think we can all agree that if a test depends on C semantics rather
than GIMPLE semantics for optimization/codegen, then removing it seems
like the right thing to do as the test is just wrong for GCC.

If the test is meant to issue a warning or avoid a false positive, then
we should defer -- I'd like to not lose the warnings if at all possible.

A test which verifies correct optimization seems like it should be
discussed.  I'd be more inclined to xfail those rather than remove them
completely -- particularly for a test which isn't a good indicator of
the real world code typically seen by gcc.

Jeff

Re: [PATCH] Make strlen range computations more conservative

[Jeff Law]

On 08/20/2018 09:15 AM, Bernd Edlinger wrote:
> On 08/20/18 16:26, Jeff Law wrote:
>> On 08/20/2018 04:23 AM, Bernd Edlinger wrote:
>>> On 08/20/18 12:12, Richard Biener wrote:
 On Wed, Aug 15, 2018 at 6:39 AM Jeff Law  wrote:
>
> On 08/10/2018 10:56 AM, Martin Sebor wrote:
>> On 08/08/2018 11:36 PM, Jeff Law wrote:
>>> On 08/02/2018 09:42 AM, Martin Sebor wrote:
>>>
 The warning bits are definitely not okay by me.  The purpose
 of the warnings (-W{format,sprintf}-{overflow,truncation} is
 to detect buffer overflows.  When a warning doesn't have access
 to string length information for dynamically created strings
 (like the strlen pass does) it uses array sizes as a proxy.
 This is useful both to detect possible buffer overflows and
 to prevent false positives for overflows that cannot happen
 in correctly written programs.
>>> So how much of this falling-back to array sizes as a proxy would become
>>> unnecessary if sprintf had access to the strlen pass as an analysis
>>> module?
>>>
>>> As you know we've been kicking that around and from my investigations
>>> that doesn't really look hard to do.  Encapsulate the data structures in
>>> a class, break up the statement handling into analysis and optimization
>>> and we should be good to go.  I'm hoping to start prototyping this week.
>>>
>>> If we think that has a reasonable chance to eliminate the array-size
>>> fallback, then that seems like the most promising path forward.
>>
>> We discussed this idea this morning so let me respond here and
>> reiterate the answer.  Using the strlen data will help detect
>> buffer overflow where the array size isn't available but it
>> cannot replace the array size heuristic. Here's a simple
>> example:
>>
>> struct S { char a[8]; };
>>
>> char d[8];
>> void f (struct S *s, int i)
>> {
>>   sprintf (d, "%s-%i",  s[i].a, i);
>> }
>>
>> We don't know the length of s->a but we do know that it can
>> be up to 7 bytes long (assuming it's nul-terminated of course)
>> so we know the sprintf call can overflow.  Conversely, if
>> the size of the destination is increased to 20  the sprintf
>> call cannot overflow so the diagnostic can be avoided.
>>
>> Removing the array size heuristic would force us to either give
>> up on diagnosing the first case or issue false positives for
>> the second case.  I think the second alternative would make
>> the warning too noisy to be useful.
>>
>> The strlen pass will help detect buffer overflows in cases
>> where the array size isn't known (e.g., with dynamically
>> allocated buffers) but where the length of the string store
>> in the array is known.  It will also help avoid false positives
>> in cases where the string stored in an array of known size is
>> known to be too short to cause an overflow.  For instance here:
>>
>> struct S { char a[8]; };
>>
>> char d[8];
>> void f (struct S *s, int i)
>> {
>>   if (strlen (s->a) < 4 && i >= 0 && i < 100)
>> sprintf (d, "%s-%i",  s->a, i);
>> }
> ACK.  Thanks for explaining things here too.  I can't speak for others,
> but seeing examples along with the explanation is easier for me to absorb.
>
> For Bernd and others -- after kicking things around a bit with Martin,
> what we're recommending is that compute_string_length we compute the
> bounds using both GIMPLE and C semantics and return both.

 But you can't do this because GIMPLE did transforms that are not valid in
 C, thus you can't interpret the GIMPLE IL as "C", you can only interpret
 it as GIMPLE.  What you'd do is return GIMPLE semantics length
 and "foobar" semantics length which doesn't match the original source.

>>>
>>> If I understood that suggestion right, it means, we
>>> live with some false positive or missing warnings due to those 
>>> transformations.
>>> That means, get_range_strlen with the 2-parameter overload is used
>>> for warnings only.  And it returns most of the time a correct range info,
>>> that is good enough for warnings.
>> Correct.  99.9% of the time using the ranges implied by the array types
>> is better for the warning code.  So the idea is to return two ranges.
>> One which uses GIMPLE semantics for code generation and optimization
>> purposes, the other which uses ranges implied by the array types for
>> warning purposes.
>>
>> Martin suggested that we always compute and return both rather than
>> having a boolean argument to select between the behavior.
>>
>>
> 
> Okay, but there is already the "strict" parameter:
[ ... ]
Sorry, I misspoke.  There's 3 ranges to return.  I believe Martin
clarified this in a subsequent message.  Sorry for any confusion.

jeff

Re: patch to bug #86829

[Joseph Myers]

On Tue, 21 Aug 2018, Jeff Law wrote:

> The problem is our VRP implementation doesn't handle any floating point
> types at this time.   If we had range information for FP types, then
> this kind of analysis is precisely what we'd need to do the
> transformation regardless of -ffast-math.

I don't think you can do it regardless of -ffast-math simply because it 
may change the semantics and we've generally assumed that if the 
optimization might produce results different from what you get with 
correctly rounded library functions, it should go under 
-funsafe-math-optimizations.  One might try to figure out a way to split 
that option, to distinguish optimizations that might change correctly 
rounded result but keep errors small from optimizations that might produce 
results that are way off, or spurious exceptions, for some inputs.

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

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

[Jeff Law]

On 08/21/2018 10:46 AM, Martin Sebor wrote:
> On 08/21/2018 09:56 AM, Jeff Law wrote:
>> On 08/21/2018 09:37 AM, Martin Sebor wrote:
>>> On 08/21/2018 05:50 AM, Joseph Myers wrote:
 On Tue, 21 Aug 2018, Martin Sebor wrote:

> I still believe it would be simpler, more robust, and safe
> to pass in a flag to either count bytes (the default, for
> all callers except sprintf %ls), or elements of the string
> type (for sprintf %ls).

 But the correct thing to count for sprintf %ls is elements of the type
 expected by sprintf %ls.  That may not be the type of the string
 constant
 (and as this is in variable arguments, there isn't any implicit
 conversion
 to const wchar_t * like you would get with a function prototype -
 I'm not
 sure if anything ensures this code is only reached, regardless of
 language, if the argument does actually (after any casts present in the
 source code) point to the correct wchar_t type).
>>>
>>> Sure, but the only valid argument to %ls is wchar_t*.  Passing
>>> it something else is undefined.
>>>
>>> The only caller interested in the element count is %ls and
>>> the only case where the function could return a result that
>>> might be different from that of C wcslen() with the same
>>> (invalid) argument is the undefined:
>>>
>>>   snprintf (0, 0, "%ls", "1234");
>>>
>>> In this case, c_strlen() would return 4 which GCC's snprintf
>>> turns into [0, 24], and calls the libc snprintf which will
>>> then proceed to read past the end of the narrow string and
>>> return some bogus value (if it doesn't crash).
>>>
>>> So I don't see a problem here(*).
>> I think we should be counting wchar_ts in this case.   If someone passes
>> something other than a wchar_t as the argument for a %ls, then the code
>> is in error.
> 
> Sure, counting wchar_t's would be fine too.  But it doesn't
> do that either -- it fails.
It fails when it encounters something unexpected (specifically when the
size of the objects being counted is not the same as the requested
count_by.  We've discussed that chunk of code and there's arguments to
keep the sanity check as well as arguments to drop the sanity check.

This can be addressed incrementally, IMHO it is not a fundamental flaw
in the patch.  How we choose to address it, either by dropping the test,
using a different function, flags, whatever we decide as engineers is
the best way to enhance the code so that we can get the diagnostics we
want.  But I think this should defer until after we figure out a plan
for the 86711/86714 codegen issues.  I was hoping to be further on that
today, but when half the day is lost to meetings, it's tough to get much
else done.

Jeff

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

[Joseph Myers]

On Tue, 21 Aug 2018, Martin Sebor wrote:

> On 08/21/2018 09:44 AM, Joseph Myers wrote:
> > On Tue, 21 Aug 2018, Martin Sebor wrote:
> > 
> > > Sure, but the only valid argument to %ls is wchar_t*.  Passing
> > > it something else is undefined.
> > 
> > Well, (wchar_t *)"something\0\0\0\0" would be OK given
> > -fno-strict-aliasing and if you know the alignment is OK.  Do we have that
> > information about the type cast to, as opposed to the type of the string
> > constant, at this point?
> 
> In the simple cases like the one above the cast is gone.  Only
> in some more involved cases is the type of the argument preserved.
> I responded to Jeff with one such example here:
> 
>   https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01296.html
> 
> If supporting (wchar_t *)"...\0\0\0\0" with %ls is viewed as
> important (despite it being undefined) then the function does

There are different cases of support.  It doesn't need to be highly 
optimized or get particularly good diagnostics.  It does need to avoid 
being miscompiled or getting actively incorrect diagnostics.

Given -fno-strict-aliasing and appropriate alignment, it's not undefined.  
(To ensure appropriate alignment one might use a target where 
BIGGEST_ALIGNMENT is 8, or one where it is 16 and a char16_t[] string 
constant is cast to pointer to 32-bit wchar_t.)

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

Re: [libiberty patch] pex-unix error behaviour

[Ian Lance Taylor via gcc-patches]

On Tue, Aug 21, 2018 at 12:46 PM, Nathan Sidwell  wrote:
> On 08/21/2018 03:04 PM, Ian Lance Taylor wrote:
>>
>> On Tue, Aug 21, 2018 at 11:15 AM, Nathan Sidwell  wrote:
>
>
>> OK, but what about a system that does
>>
>> int vfork() { return fork(); }
>>
>> ?
>
>
> Isn't such a system just buggy? Hm, apparently not.  Because of the
> requirement the user just calls 'exec(), _exit ()' a conformant program
> cannot tell whether it's fork-like or not.  However we're already violating
> that constraint by dinking environ and calling close & write [in ways that
> are ok regardless of a fork-like attribute].  I see the man page says 'The
> requirements put on vfork() by the standards are weaker than those put on
> fork(2), so an implementation where the two are synonymous is compliant.  In
> particular, the programmer cannot rely on the parent remaining blocked until
> the child either terminates or calls execve(2), and cannot rely on any
> specific behavior with respect to shared memory.'
>
> pants.  I suppose we could add an autoconf test to probe whether the child
> and parent are serialized or not.  that may or may not be simpler than ...
>
>>> The error behaviour if that program fails to be exec'd is confusing --
>>> there's an error about the exec failing but it's not attached to location
>>> information and the like, then there's a much more obvious error about
>>> communication failing.  I found it confusing the first time I tripped
>>> over
>>> it (and I was writing that bit of the compiler :)
>>
>>
>> Is there any reason we couldn't fix that without relying on the odd
>> behavior of vfork?
>
>
> Maybe, ideas?  (but this seemed the simplest way for me to get it to do what
> I wanted :)

The usual technique for better error handling is to open a pipe before
the fork, mark the pipe as close-on-exec, and have the child write
error information to the pipe.  The parent can then read from the
pipe; if it reads nothing, then the pipe was closed by the exec and
all is well.

Ian

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

[Joseph Myers]

On Tue, 21 Aug 2018, Uecker, Martin wrote:

> > I don't see why this is target-specific (if it is, the documentation for 
> > users in invoke.texi should explain what targets it works for and what it 
> > doesn't work for) anyway.  I'd expect it to be a target-independent 
> > feature with a target-independent test or tests.
> 
> My understanding is that this is a target-independent feature which
> has not yet been implemented for all targets. The existing
> documentation does not reflect this.

How does one tell which targets do or do not support it?

For tests for features supported on some but not all targets, we use 
effective-target keywords.  Of course you need to be careful about how you 
implement those keywords: you don't want the implementation of the keyword 
to be essentially the same as the test being conditioned, to avoid a bug 
in the implementation quietly causing the test to be disabled.  But the 
implementation of the keyword might e.g. have a blacklist of targets that 
do not yet support the feature, with the expectation that the test should 
run and pass on all other targets.

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

Re: patch to bug #86829

[Jeff Law]

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

 On 08/04/2018 07:22 AM, Giuliano Augusto Faulin Belinassi wrote:
> Closes bug #86829
>
> Description: Adds substitution rules for both sin(atan(x)) and
> cos(atan(x)). These formulas are replaced by x / sqrt(x*x + 1) and 1 /
> sqrt(x*x + 1) respectively, providing up to 10x speedup. This identity
> can be proved mathematically.
>
> Changelog:
>
> 2018-08-03  Giuliano Belinassi 
>
> * match.pd: add simplification rules to sin(atan(x)) and cos(atan(x)).
>
> Bootstrap and Testing:
> There were no unexpected failures in a proper testing in GCC 8.1.0
> under a x86_64 running Ubuntu 18.04.
 I understand these are mathematical identities.  But floating point
 arthmetic in a compiler isn't nearly that clean :-)  We have to worry
 about overflows, underflows, rounding, and the simple fact that many
 floating point numbers can't be exactly represented.

 Just as an example, compare the results for
 x = 0x1.fp1023

 I think sin(atan (x)) is well defined in that case.  But the x*x isn't
 because it overflows.

 So  I think this has to be somewhere under the -ffast-math umbrella.
 And the testing requirements for that are painful -- you have to verify
 it doesn't break the spec benchmark.

 I know Richi acked in the PR, but that might have been premature.
>>>
>>> It's under the flag_unsafe_math_optimizations umbrella, but sure,
>>> a "proper" way to optimize this would be to further expand
>>> sqrt (x*x + 1) to fabs(x) + ... (extra terms) that are precise enough
>>> and not have this overflow issue.
>>>
>>> But yes, I do not find (quickly skimming) other simplifications that
>>> have this kind of overflow issue (in fact I do remember raising
>>> overflow/underflow issues for other patches).
>>>
>>> Thus approval withdrawn.
>> At least until we can do some testing around spec.  There's also a patch
>> for logarithm addition/subtraction from MCC CS and another from Giuliano
>> for hyperbolics that need testing with spec.  I think that getting that
>> testing done anytime between now and stage1 close is sufficient -- none
>> of the 3 patches is particularly complex.
>>
>>
>>>
>>> If we had useful range info on floats we might conditionalize such
>>> transforms appropriately.  Or we can enable it on floats and do
>>> the sqrt (x*x + 1) in double.
>> Yea.  I keep thinking about what it might take to start doing some light
>> VRP of floating point objects.  I'd originally been thinking to just
>> track 0.0 and exceptional value state.  But the more I ponder the more I
>> think we could use the range information to allow transformations that
>> are currently guarded by the -ffast-math family of options.
>>
>> jeff
>>

Walk pointer_to and reference_to chain in free_lang_data

[Jan Hubicka]

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

Bootstrapped/regtested x86_64-linux, OK?

Honza

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

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

[Janus Weil]

Hi all,

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

The patch regtests cleanly on x86_64-linux-gnu. Ok for trunk?

Cheers,
Janus


https://github.com/janusw/gcc/commit/6f5a1b637e562b86d06d9a0d852c18ecb219c5ec
diff --git a/gcc/fortran/ChangeLog b/gcc/fortran/ChangeLog
index dc4aa1acf74..03e8b137e8f 100644
--- a/gcc/fortran/ChangeLog
+++ b/gcc/fortran/ChangeLog
@@ -1,3 +1,11 @@
+2018-08-21  Janus Weil  
+
+	PR fortran/86888
+	* decl.c (gfc_match_data_decl): Allow allocatable components of
+	indirectly recursive type.
+	* resolve.c (resolve_component): Remove two errors messages ...
+	(resolve_fl_derived): ... and replace them by a new one.
+
 2018-08-16  Nathan Sidwell  
 
 	* cpp.c (dump_macro): Use cpp_user_macro_p.
diff --git a/gcc/fortran/decl.c b/gcc/fortran/decl.c
index 1384bc717d8..03298833c98 100644
--- a/gcc/fortran/decl.c
+++ b/gcc/fortran/decl.c
@@ -5864,8 +5864,7 @@ gfc_match_data_decl (void)
   if (current_attr.pointer && gfc_comp_struct (gfc_current_state ()))
 	goto ok;
 
-  if (current_attr.allocatable && gfc_current_state () == COMP_DERIVED
-	  && current_ts.u.derived == gfc_current_block ())
+  if (current_attr.allocatable && gfc_current_state () == COMP_DERIVED)
 	goto ok;
 
   gfc_find_symbol (current_ts.u.derived->name,
diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index d65118dfae3..4ad4dcf780d 100644
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -14001,28 +14001,6 @@ resolve_component (gfc_component *c, gfc_symbol *sym)
 CLASS_DATA (c)->ts.u.derived
 = gfc_find_dt_in_generic (CLASS_DATA (c)->ts.u.derived);
 
-  if (!sym->attr.is_class && c->ts.type == BT_DERIVED && !sym->attr.vtype
-  && c->attr.pointer && c->ts.u.derived->components == NULL
-  && !c->ts.u.derived->attr.zero_comp)
-{
-  gfc_error ("The pointer component %qs of %qs at %L is a type "
- "that has not been declared", c->name, sym->name,
- &c->loc);
-  return false;
-}
-
-  if (c->ts.type == BT_CLASS && c->attr.class_ok
-  && CLASS_DATA (c)->attr.class_pointer
-  && CLASS_DATA (c)->ts.u.derived->components == NULL
-  && !CLASS_DATA (c)->ts.u.derived->attr.zero_comp
-  && !UNLIMITED_POLY (c))
-{
-  gfc_error ("The pointer component %qs of %qs at %L is a type "
- "that has not been declared", c->name, sym->name,
- &c->loc);
-  return false;
-}
-
   /* If an allocatable component derived type is of the same type as
  the enclosing derived type, we need a vtable generating so that
  the __deallocate procedure is created.  */
@@ -14258,6 +14236,13 @@ resolve_fl_derived (gfc_symbol *sym)
 			  &sym->declared_at))
 return false;
 
+  if (sym->components == NULL && !sym->attr.zero_comp)
+{
+  gfc_error ("Derived type %qs at %L has not been declared",
+		  sym->name, &sym->declared_at);
+  return false;
+}
+
   /* Resolve the finalizer procedures.  */
   if (!gfc_resolve_finalizers (sym, NULL))
 return false;
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 6c87f8017d3..5de896bdf37 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,15 @@
+2018-08-21  Janus Weil  
+
+	PR fortran/86888
+	* gfortran.dg/alloc_comp_basics_6.f90: Update an error message and add
+	an additional case.
+	* gfortran.dg/alloc_comp_basics_7.f90: New test case.
+	* gfortran.dg/class_17.f03: Update error message.
+	* gfortran.dg/class_55.f90: Ditto.
+	* gfortran.dg/dtio_11.f90: Update error messages.
+	* gfortran.dg/implicit_actual.f90: Add an error message.
+	* gfortran.dg/typebound_proc_12.f90: Update error message.
+
 2018-08-21  Marek Polacek  
 
 	PR c++/86981, Implement -Wpessimizing-move.
diff --git a/gcc/testsuite/gfortran.dg/alloc_comp_basics_6.f90 b/gcc/testsuite/gfortran.dg/alloc_comp_basics_6.f90
index 3ed221db24f..4eb0e49a7e5 100644
--- a/gcc/testsuite/gfortran.dg/alloc_comp_basics_6.f90
+++ b/gcc/testsuite/gfortran.dg/alloc_comp_basics_6.f90
@@ -5,7 +5,8 @@
 ! Contributed by Joost VandeVondele 
 
   type sysmtx_t
- type(ext_complex_t), allocatable :: S(:)  ! { dg-error "has not been previously defined" }
+ type(ext_complex_t), allocatable :: S(:)  ! { dg-error "has not been declared" }
+ class(some_type), allocatable :: X! { dg-error "has not been declared" }
   end type
 
 end
diff --git a/gcc/testsuite/gfortran.dg/alloc_comp_basics_7.f90 b/gcc/testsuite/gfortran.dg/alloc_comp_basics_7.f90
new file mode 100644
index 000..72296302169
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/alloc_comp_basics_7.f90
@@ -0,0 +1,15 @@
+! { dg-do compile }
+!
+! PR 86888: [F08] allocatable components of indirectly recursive type
+!
+! Contributed by Janus Weil 
+

Re: [C++ PATCH] Speed up inplace_merge algorithm & fix inefficient logic(PR libstdc++/83938)

[François Dumont]

I missed a test that was failing because of this patch. So I revert a 
small part of it and here is the new proposal.


Tested under Linux x86_64, ok to commit ?

François


On 24/07/2018 12:22, François Dumont wrote:

Hi

    Any chance to review this patch ?

François


On 06/06/2018 18:39, François Dumont wrote:

Hi

    I review and rework this proposal. I noticed that the same idea 
to limit buffer size within inplace_merge also apply to stable_sort.


    I also change the decision when buffer is too small to consider 
the buffer size rather than going through successive cuts of the 
original ranges. This way we won't cut the range more than necessary. 
Note that if you prefer this second part could be committed seperatly.


    PR libstdc++/83938
    * include/bits/stl_algo.h:
    (__stable_partition_adaptive): When buffer to small cut range using
    buffer size.
    (__inplace_merge): Take temporary buffer length from smallest range.
    (__merge_adaptive): When buffer too small consider smallest range 
first

    and cut based on buffer size.
    (__stable_sort_adaptive): When buffer too small cut based on buffer
    size.
    (__stable_sort): Limit temporary buffer length.
    * include/bits/stl_tempbuf.h (get_temporary_buffer): Change function
    to reduce requested buffer length on allocation failure.

Tested under Linux x86_64.

Ok to commit ?

François


On 25/01/2018 23:37, chang jc wrote:

Hi:

1. The __len = (__len + 1) / 2; is as you suggested, need to modify as
__len = (__len == 1) ? 0 : ((__len + 1) / 2);

2. The coding gain has been shown  PR c++/83938; I re-post here




   21
   20
   19
   18
   17
   16


   0.471136
   0.625695
   0.767262
   0.907461
   1.04838
   1.19508


   0.340845
   0.48651
   0.639139
   0.770133
   0.898454
   1.04632

it means when Merge [0, 4325376, 16777216); A is a sorted integer with
4325376 & B with 12451840 elements, total with 16M integers

The proposed method has the speed up under given buffer size =, ex
2^16, 2^17, ... 2^21 in unit of sizeof(int), for example, 2^16 means
given sizof(int)*64K bytes.

3. As your suggestion, _TmpBuf __buf should be rewrite.

4. It represents a fact that the intuitive idea to split from larger
part is wrong.

For example, if you have an input sorted array A & B, A has 8 integers
& B has 24 integers. Given tmp buffer whose capacity = 4 integers.

Current it tries to split from B, right?

Then we have:

A1 | A2  B1 | B2

B1 & B2 has 12 integers each, right?

Current algorithm selects pivot as 13th integer from B, right?

If the corresponding upper bound of A is 6th integer.

Then it split in

A1 = 5 | A2 = 3 | B1 = 12 | B2 = 12

After rotate, we have two arrays to merge

[A1 = 5 | B1 = 12]  & [A2 = 3 | B2 = 12]

Great, [A2 = 3 | B2 = 12] can use tmp buffer to merge.

Sadly, [A1 = 5 | B1 = 12] CANNOT.

So we do rotate again, split & merge the two split arrays from [A1 = 5
| B1 = 12] again.


But wait, if we always split from the smaller one instead of larger 
one.


After rotate, it promises two split arrays both contain 
ceiling[small/2].


Since tmp buffer also allocate by starting from sizeof(small) &
recursively downgrade by ceiling[small/2^(# of fail allocate)].

It means the allocated tmp buffer promises to be sufficient at the
level of (# of fail allocate).

Instead, you can see if split from large at level (# of fail allocate)
several split array still CANNOT use  tmp buffer to do buffered merge.


As you know, buffered merge is far speed then (split, rotate, and
merge two sub-arrays) (PR c++/83938 gives the profiling figures),

the way should provide speedup.


Thanks.










On 24/01/2018 18:23, François Dumont wrote:

Hi


 It sounds like a very sensitive change to make but nothing 
worth figures.

Do you have any bench showing the importance of the gain ?

 At least the memory usage optimization is obvious.

On 19/01/2018 10:43, chang jc wrote:

Current std::inplace_merge() suffers from performance issue by 
inefficient


logic under limited memory,

It leads to performance downgrade.

Please help to review it.

Index: include/bits/stl_algo.h
===
--- include/bits/stl_algo.h    (revision 256871)
+++ include/bits/stl_algo.h    (working copy)
@@ -2437,7 +2437,7 @@
 _BidirectionalIterator __second_cut = __middle;
 _Distance __len11 = 0;
 _Distance __len22 = 0;
-  if (__len1 > __len2)
+  if (__len1 < __len2)
   {
 __len11 = __len1 / 2;
 std::advance(__first_cut, __len11);
@@ -2539,9 +2539,15 @@
 const _DistanceType __len1 = std::distance(__first, __middle);
 const _DistanceType __len2 = std::distance(__middle, __last);

+

 typedef _Temporary_buffer<_BidirectionalIterator, _ValueType>
_TmpBuf;

-  _TmpBuf __buf(__first, __last);
-
+  _BidirectionalIterator __start, __end;
+  if (__len1 < __len2) {
+    __start = __first; __end = __middle

Re: PR libstdc++/68222 Hide safe iterator operators

[François Dumont]

On 21/08/2018 11:33, Jonathan Wakely wrote:

On 18/08/18 22:31 +0200, François Dumont wrote:
Here is the new proposal. It is indeed possible to keep 
_Safe_iterator and just add a _Category template parameter to it.


While this is still a large patch (obviously, because it's changing a
lot!) I think this version is much easier to understand, and doesn't
add a whole new class unnecessarily. Thanks for updating it.

I introduce a friend declaration to access container _Base nested 
typedef from the safe iterator.


I review the safe const_iterator constructor from safe iterator. I 
now check if we are in the a const_iterator context so that compilers 
don't even try to consider this constructor when we are in an 
iterator context.


Nice.

I adapted _Safe_local_iterator the same way to keep consistency with 
_Safe_iterator and because I find the new design cleaner. It also 
fixes the same problem I fixed on _Safe_iterator when checking it 
iterator has been initialized using _MutableIterator() rather than 
_Iterator().


I stop overloading __get_distance for safe iterators or safe local 
iterators, it was useless. I prefer to introduce similar functions as 
members. Same for __get_distance_from_begin or __get_distance_to_end, 
and I move code in safe_iterator.tcc.


Tested under Linux x86_64 debug mode.

Ok to commit ?


OK for trunk, thanks again.




Note that it also avoids to adapt the pretty printers scripts.

I'll commit it tomorrow once I'll have rework the ChangeLog entry. I'll 
also add the test case in PR 68222 adapted for the testsuite.


François

Re: patch to bug #86829

[Giuliano Augusto Faulin Belinassi]

> Just as an example, compare the results for
> x = 0x1.fp1023

Thank you for your answer and the counterexample. :-)

> If we had useful range info on floats we might conditionalize such
> transforms appropriately.  Or we can enable it on floats and do
> the sqrt (x*x + 1) in double.

I think I managed to find a bound were the transformation can be done
without overflow harm, however I don't know about rounding problems,
however

Suppose we are handling double precision floats for now. The function
x/sqrt(1 + x*x) approaches 1 when x is big enough. How big must be x
for the function be 1?

Since sqrt(1 + x*x) > x when x > 1, then we must find a value to x
that x/sqrt(1 + x*x) < eps, where eps is the biggest double smaller
than 1. Such eps must be around 1 - 2^-53 in ieee double because the
mantissa has 52 bits. Solving for x yields that x must be somewhat
bigger than 6.7e7, so let's take 1e8. Therefore if abs(x) > 1e8, it is
enough to return copysign(1, x). Notice that this arguments is also
valid for x = +-inf (if target supports that) because sin(atan(+-inf))
= +-1, and it can be extended to other floating point formats.The
following test code illustrates my point:
https://pastebin.com/M4G4neLQ

This might still be faster than calculating sin(atan(x)) explicitly.

Please let me know if this is unfeasible. :-)

Giuliano.

On Tue, Aug 21, 2018 at 11:27 AM, Jeff Law  wrote:
> On 08/21/2018 02:02 AM, Richard Biener wrote:
>> On Mon, Aug 20, 2018 at 9:40 PM Jeff Law  wrote:
>>>
>>> On 08/04/2018 07:22 AM, Giuliano Augusto Faulin Belinassi wrote:
 Closes bug #86829

 Description: Adds substitution rules for both sin(atan(x)) and
 cos(atan(x)). These formulas are replaced by x / sqrt(x*x + 1) and 1 /
 sqrt(x*x + 1) respectively, providing up to 10x speedup. This identity
 can be proved mathematically.

 Changelog:

 2018-08-03  Giuliano Belinassi 

 * match.pd: add simplification rules to sin(atan(x)) and cos(atan(x)).

 Bootstrap and Testing:
 There were no unexpected failures in a proper testing in GCC 8.1.0
 under a x86_64 running Ubuntu 18.04.
>>> I understand these are mathematical identities.  But floating point
>>> arthmetic in a compiler isn't nearly that clean :-)  We have to worry
>>> about overflows, underflows, rounding, and the simple fact that many
>>> floating point numbers can't be exactly represented.
>>>
>>> Just as an example, compare the results for
>>> x = 0x1.fp1023
>>>
>>> I think sin(atan (x)) is well defined in that case.  But the x*x isn't
>>> because it overflows.
>>>
>>> So  I think this has to be somewhere under the -ffast-math umbrella.
>>> And the testing requirements for that are painful -- you have to verify
>>> it doesn't break the spec benchmark.
>>>
>>> I know Richi acked in the PR, but that might have been premature.
>>
>> It's under the flag_unsafe_math_optimizations umbrella, but sure,
>> a "proper" way to optimize this would be to further expand
>> sqrt (x*x + 1) to fabs(x) + ... (extra terms) that are precise enough
>> and not have this overflow issue.
>>
>> But yes, I do not find (quickly skimming) other simplifications that
>> have this kind of overflow issue (in fact I do remember raising
>> overflow/underflow issues for other patches).
>>
>> Thus approval withdrawn.
> At least until we can do some testing around spec.  There's also a patch
> for logarithm addition/subtraction from MCC CS and another from Giuliano
> for hyperbolics that need testing with spec.  I think that getting that
> testing done anytime between now and stage1 close is sufficient -- none
> of the 3 patches is particularly complex.
>
>
>>
>> If we had useful range info on floats we might conditionalize such
>> transforms appropriately.  Or we can enable it on floats and do
>> the sqrt (x*x + 1) in double.
> Yea.  I keep thinking about what it might take to start doing some light
> VRP of floating point objects.  I'd originally been thinking to just
> track 0.0 and exceptional value state.  But the more I ponder the more I
> think we could use the range information to allow transformations that
> are currently guarded by the -ffast-math family of options.
>
> jeff
>

Re: [libiberty patch] pex-unix error behaviour

[Nathan Sidwell]

On 08/21/2018 03:04 PM, Ian Lance Taylor wrote:

On Tue, Aug 21, 2018 at 11:15 AM, Nathan Sidwell  wrote:



OK, but what about a system that does

int vfork() { return fork(); }

?


Isn't such a system just buggy? Hm, apparently not.  Because of the 
requirement the user just calls 'exec(), _exit ()' a conformant program 
cannot tell whether it's fork-like or not.  However we're already 
violating that constraint by dinking environ and calling close & write 
[in ways that are ok regardless of a fork-like attribute].  I see the 
man page says 'The requirements put on vfork() by the standards are 
weaker than those put on fork(2), so an implementation where the two are 
synonymous is compliant.  In particular, the programmer cannot rely on 
the parent remaining blocked until the child either terminates or calls 
execve(2), and cannot rely on any specific behavior with respect to 
shared memory.'


pants.  I suppose we could add an autoconf test to probe whether the 
child and parent are serialized or not.  that may or may not be simpler 
than ...



The error behaviour if that program fails to be exec'd is confusing --
there's an error about the exec failing but it's not attached to location
information and the like, then there's a much more obvious error about
communication failing.  I found it confusing the first time I tripped over
it (and I was writing that bit of the compiler :)


Is there any reason we couldn't fix that without relying on the odd
behavior of vfork?


Maybe, ideas?  (but this seemed the simplest way for me to get it to do 
what I wanted :)


nathan

--
Nathan Sidwell

Re: Async I/O patch with compilation fix

[Thomas Koenig]

Hi everybody,

Nicolas has committed the patch as r263750.

PR 87048 now traces the armeb regression, which is
assumed to resurface now.  Let's really try and fix
that one before 9.0 :-)

Regards

Thomas

Re: [libiberty patch] pex-unix error behaviour

[Ian Lance Taylor via gcc-patches]

On Tue, Aug 21, 2018 at 11:15 AM, Nathan Sidwell  wrote:
> On 08/21/2018 12:37 PM, Ian Lance Taylor wrote:
>>
>> On Tue, Aug 21, 2018 at 8:03 AM, Nathan Sidwell  wrote:
>>>
>>>
>>> 1) If we know we're using vfork, we can tell the parent directly via the
>>> current stack frame and suitable use of volatiles.
>>
>>
>> I'm pretty reluctant to rely on this special behavior of vfork.  A
>> system could plausibly #define vfork fork and almost all programs
>
>
> if '#define vfork fork' is in effect, we already consider it not vfork:
>
> #ifdef vfork /* Autoconf may define this to fork for us. */
> # define VFORK_STRING "fork"
> #else
> # define VFORK_STRING "vfork"
> #endif
>
> The patch continues to use that distinction.  As I mentioned, my manual
> testing (by adding such a #define), behaved as expected -- the existing
> behaviour.

OK, but what about a system that does

int vfork() { return fork(); }

?

I'm certainly willing to believe that your patch works reliably on
GNU/Linux, but this code has to be extremely portable.


>> would continue to work, but not this one.  I think we would need a
>> really compelling advantage to start doing handling vfork specially.
>> But, since errors in this code are essentially non-existent, I don't
>> see a compelling advantage here.  Is there some larger scheme this is
>> in aid of?
>
>
> On the modules branch the user can provide a mapper program, which we then
> communicate with.  So we're now execing something user-controlled, not part
> of our configuration.
>
> The error behaviour if that program fails to be exec'd is confusing --
> there's an error about the exec failing but it's not attached to location
> information and the like, then there's a much more obvious error about
> communication failing.  I found it confusing the first time I tripped over
> it (and I was writing that bit of the compiler :)

Is there any reason we couldn't fix that without relying on the odd
behavior of vfork?

Ian

Re: [libiberty patch] pex-unix error behaviour

[Nathan Sidwell]

On 08/21/2018 12:37 PM, Ian Lance Taylor wrote:

On Tue, Aug 21, 2018 at 8:03 AM, Nathan Sidwell  wrote:


1) If we know we're using vfork, we can tell the parent directly via the
current stack frame and suitable use of volatiles.


I'm pretty reluctant to rely on this special behavior of vfork.  A
system could plausibly #define vfork fork and almost all programs


if '#define vfork fork' is in effect, we already consider it not vfork:

#ifdef vfork /* Autoconf may define this to fork for us. */
# define VFORK_STRING "fork"
#else
# define VFORK_STRING "vfork"
#endif

The patch continues to use that distinction.  As I mentioned, my manual 
testing (by adding such a #define), behaved as expected -- the existing 
behaviour.



would continue to work, but not this one.  I think we would need a
really compelling advantage to start doing handling vfork specially.
But, since errors in this code are essentially non-existent, I don't
see a compelling advantage here.  Is there some larger scheme this is
in aid of?


On the modules branch the user can provide a mapper program, which we 
then communicate with.  So we're now execing something user-controlled, 
not part of our configuration.


The error behaviour if that program fails to be exec'd is confusing -- 
there's an error about the exec failing but it's not attached to 
location information and the like, then there's a much more obvious 
error about communication failing.  I found it confusing the first time 
I tripped over it (and I was writing that bit of the compiler :)


nathan
--
Nathan Sidwell

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

[Martin Sebor]

On 08/21/2018 09:44 AM, Joseph Myers wrote:

On Tue, 21 Aug 2018, Martin Sebor wrote:


Sure, but the only valid argument to %ls is wchar_t*.  Passing
it something else is undefined.


Well, (wchar_t *)"something\0\0\0\0" would be OK given
-fno-strict-aliasing and if you know the alignment is OK.  Do we have that
information about the type cast to, as opposed to the type of the string
constant, at this point?


In the simple cases like the one above the cast is gone.  Only
in some more involved cases is the type of the argument preserved.
I responded to Jeff with one such example here:

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

If supporting (wchar_t *)"...\0\0\0\0" with %ls is viewed as
important (despite it being undefined) then the function does
need an ELTSIZE argument so it knows what to count.  In that
event, in order to detect the problem cases we have been
discussing (missing nuls and mismatched argument types),
the function it must not fail when ELTSIZE is not equal
to the size of actual array element.  Instead, it needs to
return the element type to the caller which then needs to
do the validation and issue a diagnostic.

Martin

Re: VRP: rewrite the division code (to handle corner cases including 0)

[Aldy Hernandez]

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

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


Howdy!

In auditing the *_DIV_EXPR code I noticed that we were really botching
some divisions where the divisor included 0.

Particularly interesting was that we were botching something as simple
as dividing by [0,0].  We were also incorrectly calculating things like
[-2,-2] / [0, ], where we should have removed the 0 from the divisor.

Also, the symbolic special casing could be handled by just treating
symbolic ranges as [-MIN, +MAX] and letting the common code handle then.
   Similarly for anti ranges, which actually never happen except for the
constant case, since they've been normalized earlier.


Note we also have "mixed" symbolic (anti-)ranges like [0, a].  I don't think
we handled those before but extract_range_to_wide_ints may be improved
to handle them.  Likewise ranges_from_anti_range, ~[0, a] -> [-INF,
-1] u [a+1, +INF]
though not sure if that helps any case in practice.


I have added comments for a future improvements.  Perhaps after the dust 
settles...





All in all, it was much easier to normalize all the symbolic ranges and
treat everything generically by performing the division in two chunks...
the negative numbers and the (non-zero) positive numbers.  And finally,
unioning the results.  This makes everything much simpler to read with
minimal special casing.


Yeah, nice work.  Few comments:

+  TYPE_OVERFLOW_UNDEFINED (expr_type),
+  TYPE_OVERFLOW_WRAPS (expr_type),

we no longer have the third state !UNDEFINED && !WRAPS so I suggest
to eliminate one for the other (just pass TYPE_OVERFLOW_UNDEFINED).


I'm confused.  Then what shall I pass to 
wide_int_range_multiplicative_op within wide_int_range_div?  Are you 
expecting to pass overflow_undefined to both the overflow_undefined and 
overflow_wraps arguments in multiplicative_op?  Or are you saying I 
should get rid of overflow_wraps in both wide_int_range_div and 
wide_int_range_multiplicative_op (plus all other users of 
w_i_r_multiplicative_op)?




+  /* If we're definitely dividing by zero, there's nothing to do.  */
+  if (wide_int_range_zero_p (divisor_min, divisor_max, prec))
+return false;

I know we didn't do this before but for x / 0 we should compute UNDEFINED
as range.  With the current interfacing this special case would require handling
in the non-wide-int caller.


I've added the following, since I'm unsure whether we should return 
undefined or varying for non_call_exceptions.  What do you think?


  /* Special case explicit division by zero as undefined.  */
  if (range_is_null (&vr1))
{
  /* However, we must not eliminate a division by zero if
 flag_non_call_exceptions.  */
  if (cfun->can_throw_non_call_exceptions)
set_value_range_to_varying (vr);
  else
set_value_range_to_undefined (vr);
  return;
}




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


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


Ughh... Will do.  I figured someone would complain ;-).

Aldy





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


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



(Yes, we can get something like ~[0,2] for a pointer for things like the
following in libgcc:

if (segment_arg == (void *) (uintptr_type) 1)
  ...
else if (segment_arg == (void *) (uintptr_type) 2)
  return NULL;
else if (segment_arg != NULL)
  segment = (struct stack_segment *) segment_arg;
)

BTW, I am still not happy with the entire interface to wide-int-range.*,
and have another pending patchset that will simplify things even
further.  I think everyone will be pleased ;-).

OK pending another round of tests?


The division related changes are OK, please split out & improve the nonnull
parts (and the POINTER_PLUS_EXPR check is already in as variant).

Richard.



Aldy
gcc/

	* tree-vrp.c (abs_extent_range): Remove.
	(extract_range_into_wide_ints): Pass wide ints by reference.
	(extract_range_from_binary_expr_1): Rewrite the *DIV_EXPR code.
	Pass wide ints by reference in all calls to
	extract_range_into_wide_ints.
	* wide-int-range.cc (wide_int_range_div): New.
	* wide-int-range.h (wide_int_range_div): New.
	(wide_int_range_includes_zero_p): New.
	(wide_int_range_zero_p): New.

diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 24e089b019b..c56

Re: [PATCH] Optimize more boolean functions

[MCC CS]

Hello all,

I have updated the testcase and run "make check" on Ubuntu x86_64.
All of them passed. (However the last part "do-check" couldn't be
run, probably due to a missing testsuite dependency?)

The match.pd is the same as the original patch, and I have
updated change-summaries and improved tests.

2018-08-21  MCC CS  

gcc/
PR tree-optimization/87009
* match.pd: Add boolean optimizations.

2018-08-21  MCC CS  

gcc/testsuite/
PR tree-optimization/87009
* gcc.dg/pr87009.c: New test.

Index: gcc/match.pd
===
--- gcc/match.pd(revision 263646)
+++ gcc/match.pd(working copy)
@@ -776,6 +776,11 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
  (bit_not (bit_and:cs (bit_not @0) @1))
  (bit_ior @0 (bit_not @1)))
 
+/* ~(~a | b)  -->  a & ~b  */
+(simplify
+ (bit_not (bit_ior:cs (bit_not @0) @1))
+ (bit_and @0 (bit_not @1)))
+
 /* Simplify (~X & Y) to X ^ Y if we know that (X & ~Y) is 0.  */
 #if GIMPLE
 (simplify
@@ -981,6 +986,16 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
  (bit_and:c (bit_ior:c @0 @1) (bit_xor:c @1 (bit_not @0)))
  (bit_and @0 @1))
 
+/* (~x | y) & (x | ~y) -> ~(x ^ y) */
+(simplify
+ (bit_and (bit_ior:c (bit_not @0) @1) (bit_ior:c @0 (bit_not @1)))
+ (bit_not (bit_xor @0 @1)))
+
+/* (~x | y) ^ (x | ~y) -> x ^ y */
+(simplify
+ (bit_xor (bit_ior:c (bit_not @0) @1) (bit_ior:c @0 (bit_not @1)))
+ (bit_xor @0 @1))
+
 /* ~x & ~y -> ~(x | y)
~x | ~y -> ~(x & y) */
 (for op (bit_and bit_ior)
Index: gcc/testsuite/gcc.dg/pr87009.c
===
--- gcc/testsuite/gcc.dg/pr87009.c  (nonexistent)
+++ gcc/testsuite/gcc.dg/pr87009.c  (working copy)
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-original" } */
+/* { dg-final { scan-tree-dump-times "return s \\^ x;" 4 "original" } } */
+
+int f1 (int x, int s)
+{
+  return ~(~(x|s)|x)|~(~(x|s)|s);
+}
+
+int f2 (int x, int s)
+{
+  return ~(~(~x&s)&~(x&~s));
+}
+
+int f3 (int x, int s)
+{
+  return ~((x|~s)&(~x|s));
+}
+
+int f4 (int x, int s)
+{
+  return (x|~s)^(~x|s);
+}

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

[Martin Sebor]

On 08/21/2018 09:56 AM, Jeff Law wrote:

On 08/21/2018 09:37 AM, Martin Sebor wrote:

On 08/21/2018 05:50 AM, Joseph Myers wrote:

On Tue, 21 Aug 2018, Martin Sebor wrote:


I still believe it would be simpler, more robust, and safe
to pass in a flag to either count bytes (the default, for
all callers except sprintf %ls), or elements of the string
type (for sprintf %ls).


But the correct thing to count for sprintf %ls is elements of the type
expected by sprintf %ls.  That may not be the type of the string constant
(and as this is in variable arguments, there isn't any implicit
conversion
to const wchar_t * like you would get with a function prototype - I'm not
sure if anything ensures this code is only reached, regardless of
language, if the argument does actually (after any casts present in the
source code) point to the correct wchar_t type).


Sure, but the only valid argument to %ls is wchar_t*.  Passing
it something else is undefined.

The only caller interested in the element count is %ls and
the only case where the function could return a result that
might be different from that of C wcslen() with the same
(invalid) argument is the undefined:

  snprintf (0, 0, "%ls", "1234");

In this case, c_strlen() would return 4 which GCC's snprintf
turns into [0, 24], and calls the libc snprintf which will
then proceed to read past the end of the narrow string and
return some bogus value (if it doesn't crash).

So I don't see a problem here(*).

I think we should be counting wchar_ts in this case.   If someone passes
something other than a wchar_t as the argument for a %ls, then the code
is in error.


Sure, counting wchar_t's would be fine too.  But it doesn't
do that either -- it fails.


At issue is we need a way for the sprintf code to indicate to c_strlen
the size of a wchar_t argument.  Between Joseph's suggestions and my own
poking around I think we can do that.  I just have to fight aix long
enough to be able to do some testing.



In any event, my main concern isn't whether the function take
a flag or an element size as an argument.  What I believe it
needs to do is count bytes when requested even when the array
is a wide string.

Umm, that's precisely what it does.  If you ask it to count bytes, it'll
count bytes.  You can request to count 16bit wchat_t and a 32bit wchar_t
objects.  So I'm not sure why we're still arguing over this.


No, that's not what the function does.

It fails when the request doesn't match the type of the array
element: i.e, it fails when passed a wchar_t array and ELTSIZE
== 1.  This is because of the following test:

  /* Determine the size of the string element.  */
  if (eltsize != tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE 
(src)

return NULL_TREE;

This prevents diagnosing the interesting cases that we have been
discussing here (missing nuls and the type mismatches).

Martin

Re: [libiberty patch] pex-unix error behaviour

[Ian Lance Taylor via gcc-patches]

On Tue, Aug 21, 2018 at 8:03 AM, Nathan Sidwell  wrote:
>
> 1) If we know we're using vfork, we can tell the parent directly via the
> current stack frame and suitable use of volatiles.

I'm pretty reluctant to rely on this special behavior of vfork.  A
system could plausibly #define vfork fork and almost all programs
would continue to work, but not this one.  I think we would need a
really compelling advantage to start doing handling vfork specially.
But, since errors in this code are essentially non-existent, I don't
see a compelling advantage here.  Is there some larger scheme this is
in aid of?

Ian

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

[Martin Sebor]

On 08/21/2018 09:59 AM, Jeff Law wrote:

On 08/21/2018 09:57 AM, Martin Sebor wrote:

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

On Tue, 21 Aug 2018, Bernd Edlinger wrote:


gcc -S -O2 -Wall -Wformat-overflow -ftrack-macro-expansion=0
-fshort-wchar builtin-sprintf-warn-20.c
builtin-sprintf-warn-20.c: In function 'test':
builtin-sprintf-warn-20.c:19:39: warning: hex escape sequence out of
range
19 | ? (char*)L"\x4142\x4344" : (char*)L"\x41424344\x45464748";
|   ^~~

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

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

For constant objects like those, the GIMPLE type is still guaranteed
to be reliable,
right?


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


A warning for these cases should be relatively  straightforward
to add to the sprintf pass.  It would require c_strlen() to return
the type of the string argument to the caller.  That way sprintf's
format_string() function could compare the string type to the
expected type of the directive.

Umm, why would c_strlen need to return that?  We should be able to get
to it directly from the argument?!?  What am I missing here?


The sprintf pass sees the type of the argument.  In the literal
case above the cast is folded away and so sprintf does see
the type of the argument.  In more interesting cases like
the one below it sees the type of the argument (i.e., wchar_t*)
but c_strlen() has the smarts to get at the underlying object:

  const char a[] = "1234";

  int f (int i)
  {
__WCHAR_TYPE__ *p = a;

return __builtin_snprintf (0, 0, "%ls", &p[i]);
  }

Martin

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

[Martin Sebor]

On 08/21/2018 09:27 AM, Jeff Law wrote:

On 08/20/2018 11:17 PM, Bernd Edlinger wrote:

On 08/21/18 01:23, Martin Sebor wrote:

On 08/20/2018 04:17 PM, Jeff Law wrote:

On 08/18/2018 11:38 AM, Martin Sebor wrote:

On 08/17/2018 09:32 PM, Jeff Law wrote:

On 08/17/2018 02:17 PM, Martin Sebor wrote:

On 08/17/2018 12:44 PM, Bernd Edlinger wrote:

On 08/17/18 20:23, Martin Sebor wrote:

On 08/17/2018 06:14 AM, Joseph Myers wrote:

On Fri, 17 Aug 2018, Jeff Law wrote:


On 08/16/2018 05:01 PM, Joseph Myers wrote:

On Thu, 16 Aug 2018, Jeff Law wrote:


restores previous behavior.  The sprintf bits want to count
element
sized chunks, which for wchars is 4 bytes (that count will then be



   /* Compute the range the argument's length can be in.  */
-  fmtresult slen = get_string_length (arg);
+  int count_by = dir.specifier == 'S' || dir.modifier ==
FMT_LEN_l ? 4 : 1;


I don't see how a hardcoded 4 is correct here.  Surely you need to
example
wchar_type_node to determine its actual size for this target.

We did kick this around a little.  IIRC Martin didn't think that it
was
worth handling the 2 byte wchar case.


Sorry, I think we may have miscommunicated -- I didn't think it
was useful to pass a size of the character type to the function.
I agree that passing in a hardcoded constant doesn't seem right
(even if GCC's wchar_t were always 4 bytes wide).

I'm still not sure I see the benefit of passing in the expected
element size given that the patch causes c_strlen() fail when
the actual element size doesn't match ELTSIZE.  If the caller
asks for the number of bytes in a const wchar_t array it should
get back the number bytes.  (I could see it fail if the caller
asked for the number of words in a char array whose size was
not evenly divisible by wordsize.)



I think in this case c_strlen should use the type which the %S format
uses at runtime, otherwise it will not have anything to do with
the reality.


%S is not what I'm talking about.

Failing in the case I described (a caller asking for the size
in bytes of a constant object whose type is larger) prevents
callers that don't care about the type from detecting the actual
size of a constant.

Specifically for sprintf, it means that the buffer overflow
below is no longer diagnosed:

  struct S { char a[2]; void (*pf)(void); };

  void f (struct S *p)
  {
const char *q = (char*)L"\x41424344\x45464748";

sprintf (p->a, "%s", q);
  }

I don't think this is in the testsuite, is it?  I verified that there
was no regressions when I installed Bernd's patch and when I installed
yours.


No, there are very few tests that exercise these kinds of mixed
argument types.  Code like that is most likely the result of
a mistake, but it's not the kind of a bug I had even thought
about until some of the codegen issues with mixed argument types
were brought up (PR 86711/86714).

Phew.  I was worried I'd somehow missed the failure or tested the wrong
patch or who knows what.

Can you add that test, xfailed for now to the testsuite?


Done in r263676.


I would just like the ability to get the length/size somehow
so that the questionable code that started us down this path
can be detected.  This is not just the sprintf(d, "%s", L"1")
kind of a mismatch but also the missing nul detection in cases
like:

[ ... ]
I know.  And ideally we'll be able to handle everything you want to.
But we also have to realize that sometimes we may have to punt.

Also remember that incremental progress is (usually) good.




  const wchar_t a[1] = L"\x";
  strcpy(d, (char*)a);

This touches on both the representational issue (excess elements in the
initializer) and how to handle a non-terminated string.  Both are issues
we're debating right now.



I don't think this is necessarily an important use case to
optimize for but it is one that GCC optimizes already nonetheless,
and always has.  For example, this has always been folded into 4
by GCC and still is even with the patch:

  const __WCHAR_TYPE__ wc[] = L"\x12345678";

  int f (void)
  {
const char *p = (char*)wc;
return strlen (p);// folded to 4
  }

It's optimized because fold_const_call() relies on c_getstr()
which returns the underlying byte representation of the wide
string, and despite c_strlen() now trying to prevent it.

And I think you're hitting on some of issues already raised in the thread.

In this specific case though ISTM 4 is the right answer.  We casted to a
char *, so that's what we should be counting.  Or am I missing
something?  Also note that's the value we'd get from the strlen C
library call IIUC.


It is the right answer.  My point is that this is optimized
but the change to c_strlen() prevents the same optimization
in other similar cases.  For example, GCC 6 folds this into
memcpy:

   __builtin_strcpy (d, (char*)L"\x12345678");

GCC 7 and 8 do too but get the byte count wrong (my bad).

Current trunk doesn't optimize it.  If restoring the original
behavior is the intent (and not just fix

[PATCH] Optimise sqrt reciprocal multiplications

[Kyrill Tkachov]

Hi all,

This patch aims to optimise sequences involving uses of 1.0 / sqrt (a) under 
-freciprocal-math and -funsafe-math-optimizations.
In particular consider:

x = 1.0 / sqrt (a);
r1 = x * x;  // same as 1.0 / a
r2 = a * x; // same as sqrt (a)

If x, r1 and r2 are all used further on in the code, this can be transformed 
into:
tmp1 = 1.0 / a
tmp2 = sqrt (a)
tmp3 = tmp1 * tmp2
x = tmp3
r1 = tmp1
r2 = tmp2

A bit convoluted, but this saves us one multiplication and, more importantly, 
the sqrt and division are now independent.
This also allows optimisation of a subset of these expressions.
For example:
x = 1.0 / sqrt (a)
r1 = x * x

can be transformed to r1 = 1.0 / a, eliminating the sqrt if x is not used 
anywhere else.
And similarly:
x = 1.0 / sqrt (a)
r1 = a * x

can be transformed to sqrt (a) eliminating the division.

For the testcase:
double res, res2, tmp;
void
foo (double a, double b)
{
  tmp = 1.0 / __builtin_sqrt (a);
  res = tmp * tmp;
  res2 = a * tmp;
}

We now generate for aarch64 with -Ofast:
foo:
fmovd2, 1.0e+0
adrpx2, res2
fsqrt   d1, d0
adrpx1, res
fdivd0, d2, d0
adrpx0, tmp
str d1, [x2, #:lo12:res2]
fmuld1, d1, d0
str d0, [x1, #:lo12:res]
str d1, [x0, #:lo12:tmp]
ret

where before it generated:
foo:
fsqrt   d2, d0
fmovd1, 1.0e+0
adrpx1, res2
adrpx2, tmp
adrpx0, res
fdivd1, d1, d2
fmuld0, d1, d0
fmuld2, d1, d1
str d1, [x2, #:lo12:tmp]
str d0, [x1, #:lo12:res2]
str d2, [x0, #:lo12:res]
ret

As you can see, the new sequence has one fewer multiply and the fsqrt and fdiv 
are independent.

With this patch I see a 14% improvement on 544.nab_r from SPEC2017 on 
Cortex-A72 at -Ofast.
Bootstrapped and tested on aarch64-none-linux-gnu and x86_64-unknown-linux-gnu.

Ok for trunk?
Thanks,
Kyrill

2018-08-21  Kyrylo Tkachov  

* tree-ssa-math-opts.c (is_mult_by): New function.
(optimize_recip_sqrt): Likewise.
(pass_cse_reciprocals::execute): Use the above.

2018-08-21  Kyrylo Tkachov  

* gcc.dg/recip_sqrt_mult_1.c: New test.
* gcc.dg/recip_sqrt_mult_2.c: Likewise.
* gcc.dg/recip_sqrt_mult_3.c: Likewise.
commit ea11c9eb018abf4e21c61b8a7305291b0d9a7ec8
Author: kyrtka01 
Date:   Tue Aug 21 13:54:07 2018 +0100

Optimise sqrt reciprocal multiplications

diff --git a/gcc/testsuite/gcc.dg/recip_sqrt_mult_1.c b/gcc/testsuite/gcc.dg/recip_sqrt_mult_1.c
new file mode 100644
index 000..86429e4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/recip_sqrt_mult_1.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-Ofast -fdump-tree-recip" } */
+
+double res, res2, tmp;
+void
+foo (double a, double b)
+{
+  tmp = 1.0 / __builtin_sqrt (a);
+  res = tmp * tmp;
+  res2 = a * tmp;
+}
+
+/* { dg-final { scan-tree-dump "Optimizing reciprocal sqrt multiplications" "recip" } } */
+/* { dg-final { scan-tree-dump "Replacing squaring multiplication" "recip" } } */
+/* { dg-final { scan-tree-dump "Replacing multiplication" "recip" } } */
diff --git a/gcc/testsuite/gcc.dg/recip_sqrt_mult_2.c b/gcc/testsuite/gcc.dg/recip_sqrt_mult_2.c
new file mode 100644
index 000..c5fc3de
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/recip_sqrt_mult_2.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-Ofast -fdump-tree-optimized" } */
+
+float
+foo (float a)
+{
+  float tmp = 1.0f / __builtin_sqrtf (a);
+  return a * tmp;
+}
+
+/* { dg-final { scan-tree-dump-not " / " "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/recip_sqrt_mult_3.c b/gcc/testsuite/gcc.dg/recip_sqrt_mult_3.c
new file mode 100644
index 000..e7d185b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/recip_sqrt_mult_3.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-Ofast -fdump-tree-optimized" } */
+
+double
+foo (double a)
+{
+  double tmp = 1.0f / __builtin_sqrt (a);
+  return tmp * tmp;
+}
+
+/* { dg-final { scan-tree-dump-not "__builtin_sqrt" "optimized" } } */
diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c
index a90d9d2..b25beaf 100644
--- a/gcc/tree-ssa-math-opts.c
+++ b/gcc/tree-ssa-math-opts.c
@@ -352,6 +352,23 @@ is_square_of (gimple *use_stmt, tree def)
   return 0;
 }
 
+/* Return TRUE if USE_STMT is a multiplication of DEF by A.  */
+
+static inline bool
+is_mult_by (gimple *use_stmt, tree def, tree a)
+{
+  if (gimple_code (use_stmt) == GIMPLE_ASSIGN
+  && gimple_assign_rhs_code (use_stmt) == MULT_EXPR)
+{
+  tree op0 = gimple_assign_rhs1 (use_stmt);
+  tree op1 = gimple_assign_rhs2 (use_stmt);
+
+  return (op0 == def && op1 == a)
+	  || (op0 == a && op1 == def);
+}
+  return 0;
+}
+
 /* Return whether USE_STMT is a floating-point division by
DEF * DEF.  */
 static inline bool
@@ -652,6 +669,180 @@ execute_cse_reciprocals_1 (gimple_stmt_iterator *def_gsi, tree def)
   occ_head = NULL;
 }
 
+/* Transf

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

[Jeff Law]

On 08/21/2018 09:57 AM, Martin Sebor wrote:
> On 08/21/2018 02:59 AM, Richard Biener wrote:
>> On Tue, 21 Aug 2018, Bernd Edlinger wrote:
>>
>>> gcc -S -O2 -Wall -Wformat-overflow -ftrack-macro-expansion=0
>>> -fshort-wchar builtin-sprintf-warn-20.c
>>> builtin-sprintf-warn-20.c: In function 'test':
>>> builtin-sprintf-warn-20.c:19:39: warning: hex escape sequence out of
>>> range
>>> 19 | ? (char*)L"\x4142\x4344" : (char*)L"\x41424344\x45464748";
>>>     |   ^~~
>>>
>>> Hmm, this test might create some noise on short-wchar targets.
>>>
>>> I would prefer a warning here, about the wrong type of the parameter.
>>> The buffer overflow is only a secondary thing.
>>>
>>> For constant objects like those, the GIMPLE type is still guaranteed
>>> to be reliable,
>>> right?
>>
>> TREE_TYPE of tcc_declaration and tcc_constant trees should more-or-less
>> (minus qualifications not affecting semantics) be those set by
>> frontends.
> 
> A warning for these cases should be relatively  straightforward
> to add to the sprintf pass.  It would require c_strlen() to return
> the type of the string argument to the caller.  That way sprintf's
> format_string() function could compare the string type to the
> expected type of the directive.
Umm, why would c_strlen need to return that?  We should be able to get
to it directly from the argument?!?  What am I missing here?

jeff

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

[Martin Sebor]

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

On Tue, 21 Aug 2018, Bernd Edlinger wrote:


gcc -S -O2 -Wall -Wformat-overflow -ftrack-macro-expansion=0 -fshort-wchar 
builtin-sprintf-warn-20.c
builtin-sprintf-warn-20.c: In function 'test':
builtin-sprintf-warn-20.c:19:39: warning: hex escape sequence out of range
19 | ? (char*)L"\x4142\x4344" : (char*)L"\x41424344\x45464748";
|   ^~~

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

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

For constant objects like those, the GIMPLE type is still guaranteed to be 
reliable,
right?


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


A warning for these cases should be relatively  straightforward
to add to the sprintf pass.  It would require c_strlen() to return
the type of the string argument to the caller.  That way sprintf's
format_string() function could compare the string type to the
expected type of the directive.

If c_strlen() fails because the types don't match as it does now
there's no easy way for format_string() to know what the actual
type of the string is (not without doing the same work c_strlen()
would have done).

Martin

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

[Jeff Law]

On 08/21/2018 09:37 AM, Martin Sebor wrote:
> On 08/21/2018 05:50 AM, Joseph Myers wrote:
>> On Tue, 21 Aug 2018, Martin Sebor wrote:
>>
>>> I still believe it would be simpler, more robust, and safe
>>> to pass in a flag to either count bytes (the default, for
>>> all callers except sprintf %ls), or elements of the string
>>> type (for sprintf %ls).
>>
>> But the correct thing to count for sprintf %ls is elements of the type
>> expected by sprintf %ls.  That may not be the type of the string constant
>> (and as this is in variable arguments, there isn't any implicit
>> conversion
>> to const wchar_t * like you would get with a function prototype - I'm not
>> sure if anything ensures this code is only reached, regardless of
>> language, if the argument does actually (after any casts present in the
>> source code) point to the correct wchar_t type).
> 
> Sure, but the only valid argument to %ls is wchar_t*.  Passing
> it something else is undefined.
> 
> The only caller interested in the element count is %ls and
> the only case where the function could return a result that
> might be different from that of C wcslen() with the same
> (invalid) argument is the undefined:
> 
>   snprintf (0, 0, "%ls", "1234");
> 
> In this case, c_strlen() would return 4 which GCC's snprintf
> turns into [0, 24], and calls the libc snprintf which will
> then proceed to read past the end of the narrow string and
> return some bogus value (if it doesn't crash).
> 
> So I don't see a problem here(*).
I think we should be counting wchar_ts in this case.   If someone passes
something other than a wchar_t as the argument for a %ls, then the code
is in error.

At issue is we need a way for the sprintf code to indicate to c_strlen
the size of a wchar_t argument.  Between Joseph's suggestions and my own
poking around I think we can do that.  I just have to fight aix long
enough to be able to do some testing.

> 
> In any event, my main concern isn't whether the function take
> a flag or an element size as an argument.  What I believe it
> needs to do is count bytes when requested even when the array
> is a wide string.
Umm, that's precisely what it does.  If you ask it to count bytes, it'll
count bytes.  You can request to count 16bit wchat_t and a 32bit wchar_t
objects.  So I'm not sure why we're still arguing over this.




> 
> Martin
> 
> PS The only problem I see is that GCC will call the library
> function in this case, which we know is undefined and will
> unavoidably read past the end of the argument and (if it
> doesn't crash due to misalignment) return some indeterminate
> value.  That's worse than any other outcome (such as folding
> the call into a constant or trapping) and can easily be
> prevented.  But that's a separate discussion.
Right.  So let's please defer it.  We don't need to expand this
discussion further, it's tangled enough already.

jeff

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

[Joseph Myers]

On Tue, 21 Aug 2018, Martin Sebor wrote:

> Sure, but the only valid argument to %ls is wchar_t*.  Passing
> it something else is undefined.

Well, (wchar_t *)"something\0\0\0\0" would be OK given 
-fno-strict-aliasing and if you know the alignment is OK.  Do we have that 
information about the type cast to, as opposed to the type of the string 
constant, at this point?

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

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

[Martin Sebor]

On 08/21/2018 05:50 AM, Joseph Myers wrote:

On Tue, 21 Aug 2018, Martin Sebor wrote:


I still believe it would be simpler, more robust, and safe
to pass in a flag to either count bytes (the default, for
all callers except sprintf %ls), or elements of the string
type (for sprintf %ls).


But the correct thing to count for sprintf %ls is elements of the type
expected by sprintf %ls.  That may not be the type of the string constant
(and as this is in variable arguments, there isn't any implicit conversion
to const wchar_t * like you would get with a function prototype - I'm not
sure if anything ensures this code is only reached, regardless of
language, if the argument does actually (after any casts present in the
source code) point to the correct wchar_t type).


Sure, but the only valid argument to %ls is wchar_t*.  Passing
it something else is undefined.

The only caller interested in the element count is %ls and
the only case where the function could return a result that
might be different from that of C wcslen() with the same
(invalid) argument is the undefined:

  snprintf (0, 0, "%ls", "1234");

In this case, c_strlen() would return 4 which GCC's snprintf
turns into [0, 24], and calls the libc snprintf which will
then proceed to read past the end of the narrow string and
return some bogus value (if it doesn't crash).

So I don't see a problem here(*).

In any event, my main concern isn't whether the function take
a flag or an element size as an argument.  What I believe it
needs to do is count bytes when requested even when the array
is a wide string.

Martin

PS The only problem I see is that GCC will call the library
function in this case, which we know is undefined and will
unavoidably read past the end of the argument and (if it
doesn't crash due to misalignment) return some indeterminate
value.  That's worse than any other outcome (such as folding
the call into a constant or trapping) and can easily be
prevented.  But that's a separate discussion.

Re: [PATCH, testsuite] Stringify __USER_LABEL_PREFIX__ in pr85248.

[Mike Stump]

On Aug 21, 2018, at 7:20 AM, Iain Sandoe  wrote:
> 
> another missing stringify for _U_L_P_ 
> 
> OK?

Ok.
=

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

[Jeff Law]

On 08/20/2018 11:17 PM, Bernd Edlinger wrote:
> On 08/21/18 01:23, Martin Sebor wrote:
>> On 08/20/2018 04:17 PM, Jeff Law wrote:
>>> On 08/18/2018 11:38 AM, Martin Sebor wrote:
 On 08/17/2018 09:32 PM, Jeff Law wrote:
> On 08/17/2018 02:17 PM, Martin Sebor wrote:
>> On 08/17/2018 12:44 PM, Bernd Edlinger wrote:
>>> On 08/17/18 20:23, Martin Sebor wrote:
 On 08/17/2018 06:14 AM, Joseph Myers wrote:
> On Fri, 17 Aug 2018, Jeff Law wrote:
>
>> On 08/16/2018 05:01 PM, Joseph Myers wrote:
>>> On Thu, 16 Aug 2018, Jeff Law wrote:
>>>
 restores previous behavior.  The sprintf bits want to count
 element
 sized chunks, which for wchars is 4 bytes (that count will then be
>>>
    /* Compute the range the argument's length can be in.  */
 -  fmtresult slen = get_string_length (arg);
 +  int count_by = dir.specifier == 'S' || dir.modifier ==
 FMT_LEN_l ? 4 : 1;
>>>
>>> I don't see how a hardcoded 4 is correct here.  Surely you need to
>>> example
>>> wchar_type_node to determine its actual size for this target.
>> We did kick this around a little.  IIRC Martin didn't think that it
>> was
>> worth handling the 2 byte wchar case.

 Sorry, I think we may have miscommunicated -- I didn't think it
 was useful to pass a size of the character type to the function.
 I agree that passing in a hardcoded constant doesn't seem right
 (even if GCC's wchar_t were always 4 bytes wide).

 I'm still not sure I see the benefit of passing in the expected
 element size given that the patch causes c_strlen() fail when
 the actual element size doesn't match ELTSIZE.  If the caller
 asks for the number of bytes in a const wchar_t array it should
 get back the number bytes.  (I could see it fail if the caller
 asked for the number of words in a char array whose size was
 not evenly divisible by wordsize.)

>>>
>>> I think in this case c_strlen should use the type which the %S format
>>> uses at runtime, otherwise it will not have anything to do with
>>> the reality.
>>
>> %S is not what I'm talking about.
>>
>> Failing in the case I described (a caller asking for the size
>> in bytes of a constant object whose type is larger) prevents
>> callers that don't care about the type from detecting the actual
>> size of a constant.
>>
>> Specifically for sprintf, it means that the buffer overflow
>> below is no longer diagnosed:
>>
>>   struct S { char a[2]; void (*pf)(void); };
>>
>>   void f (struct S *p)
>>   {
>>     const char *q = (char*)L"\x41424344\x45464748";
>>
>>     sprintf (p->a, "%s", q);
>>   }
> I don't think this is in the testsuite, is it?  I verified that there
> was no regressions when I installed Bernd's patch and when I installed
> yours.

 No, there are very few tests that exercise these kinds of mixed
 argument types.  Code like that is most likely the result of
 a mistake, but it's not the kind of a bug I had even thought
 about until some of the codegen issues with mixed argument types
 were brought up (PR 86711/86714).
>>> Phew.  I was worried I'd somehow missed the failure or tested the wrong
>>> patch or who knows what.
>>>
>>> Can you add that test, xfailed for now to the testsuite?
>>
>> Done in r263676.
>>
 I would just like the ability to get the length/size somehow
 so that the questionable code that started us down this path
 can be detected.  This is not just the sprintf(d, "%s", L"1")
 kind of a mismatch but also the missing nul detection in cases
 like:
>>> [ ... ]
>>> I know.  And ideally we'll be able to handle everything you want to.
>>> But we also have to realize that sometimes we may have to punt.
>>>
>>> Also remember that incremental progress is (usually) good.
>>>
>>>

   const wchar_t a[1] = L"\x";
   strcpy(d, (char*)a);
>>> This touches on both the representational issue (excess elements in the
>>> initializer) and how to handle a non-terminated string.  Both are issues
>>> we're debating right now.
>>>

 I don't think this is necessarily an important use case to
 optimize for but it is one that GCC optimizes already nonetheless,
 and always has.  For example, this has always been folded into 4
 by GCC and still is even with the patch:

   const __WCHAR_TYPE__ wc[] = L"\x12345678";

   int f (void)
   {
     const char *p = (char*)wc;
     return strlen (p);    // folded to 4
   }

 It's optimized because fold_const_call() relies on c_getstr()
 which returns the underlying byte representation of the wide
 string, and despite c_strlen() now t

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

[Jeff Law]

On 08/20/2018 05:23 PM, Martin Sebor wrote:
>> And I think you're hitting on some of issues already raised in the
>> thread.
>>
>> In this specific case though ISTM 4 is the right answer.  We casted to a
>> char *, so that's what we should be counting.  Or am I missing
>> something?  Also note that's the value we'd get from the strlen C
>> library call IIUC.
> 
> It is the right answer.  My point is that this is optimized
> but the change to c_strlen() prevents the same optimization
> in other similar cases.  For example, GCC 6 folds this into
> memcpy:
> 
>   __builtin_strcpy (d, (char*)L"\x12345678");
> 
> GCC 7 and 8 do too but get the byte count wrong (my bad).
> 
> Current trunk doesn't optimize it.  If restoring the original
> behavior is the intent (and not just fixing the counting bug)
> then c_strlen() should be fixed to fold this again.
> 
> (I'm not a fan of the strcpy to memcpy transformation because
> it makes other things difficult but that's beside the point.)
I care a whole lot less about optimization of a wide character strcpy
than I do about the larger issues around correctness or getting good
warnings.

Again, if you want to add these things to the testsuite as well (xfail'd
of course), that's fine.


> 
>>> The missing nul variant of the same test case isn't folded (it
>>> ends up calling the library strlen) but the bug cannot be
>>> detected using the modified c_strlen():
>>>
>>>   const __WCHAR_TYPE__ wc[1] = L"\x12345678";   // no nul
>>>
>>>   int f (void)
>>>   {
>>>     const char *p = (char*)wc;
>>>     return strlen (p);    // not folded
>>>   }
>>>
>>> To detect it, I'd have to use some other function, like
>>> c_getstr().  I could certainly do that but I don't think
>>> I should need to.
>> And that's on the list of things we're going to try and address next.  I
>> don't think it needs to be conflated with change which indicated to
>> c_strlen how to count.
> 
> The two are one and the same: if c_strlen() doesn't count bytes
> in arrays of wide characters (wchar_t, char16_t, or char32_t)
> then the original GCC behavior is not restored and the missing
> nul detection won't work for these "mixed type" cases.
My understanding is prior to the work in gcc-7/gcc-8 c_strlen always
counted in bytes, regardless of the incoming type.  That was, IMHO,
proper behavior.  In retrospect I should have caught the change to have
it count elements before it went in.

I'm a whole lot less concernd that we're missing an optimization on wide
character types than I am about restoring the behavior of c_strlen.

Optimizing for wide char types, can wait and when addressed need not do
so through c_strlen.


> 
>> I do hope you're getting all these testcases recorded.  I'd even support
>> installing them into the suite now, properly xfailed.  It's easier to
>> see how any patch under consideration affects the wider set of tests.
> 
> I haven't been recording any of them --  I have no idea where
> this is going.  As I've said, these patches prevent some of
> the work I have already submitted.  That's why I have been
> objecting to them, including adding the ELTSIZE argument to
> c_strlen().
I don't know where all the ends either.  It is certainly a possibility
that some of the already submitted work by you or Bernd will need to be
reworked and that some my be dropped.  It happens to all of us from time
to time.

The issues that have been raised in these threads are significant and
need to be resolved.

Jeff

C++ PATCH to fix typo in cp-tree.h

[Marek Polacek]

We don't have INDIRECT_EXPR.

Applying as obvious.

2018-08-21  Marek Polacek  

* cp-tree.h: Fix typo.

--- gcc/cp/cp-tree.h
+++ gcc/cp/cp-tree.h
@@ -3656,7 +3656,7 @@ struct GTY(()) lang_decl {
   (LANG_DECL_FN_CHECK (FUNCTION_DECL_CHECK (NODE)) \
->u.saved_language_function)
 
-/* True if NODE is an implicit INDIRECT_EXPR from convert_from_reference.  */
+/* True if NODE is an implicit INDIRECT_REF from convert_from_reference.  */
 #define REFERENCE_REF_P(NODE)  \
   (INDIRECT_REF_P (NODE)   \
&& TREE_TYPE (TREE_OPERAND (NODE, 0))   \

Re: C++ PATCH for c++/86981, implement -Wpessimizing-move

[Marek Polacek]

On Tue, Aug 21, 2018 at 04:41:52PM +1200, Jason Merrill wrote:
> Let's factor this into a maybe_warn_pessimizing_move function. Ok with that
> change.

Thanks for the quick review.  Thus:

Bootstrapped/regtested on x86_64-linux, applying to trunk.

2018-08-21  Marek Polacek  

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

* typeck.c (decl_in_std_namespace_p): New.
(is_std_move_p): New.
(maybe_warn_pessimizing_move): New.
(can_do_nrvo_p): New, factored out of ...
(check_return_expr): ... here.  Warn about potentially harmful
std::move in a return statement.

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

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

diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index 9980bfac11c..76840dd77ad 100644
--- gcc/c-family/c.opt
+++ gcc/c-family/c.opt
@@ -937,6 +937,10 @@ Wpedantic
 C ObjC C++ ObjC++ CPP(cpp_pedantic) CppReason(CPP_W_PEDANTIC) Warning
 ; Documented in common.opt
 
+Wpessimizing-move
+C++ ObjC++ Var(warn_pessimizing_move) Warning LangEnabledBy(C++ ObjC++, Wall)
+Warn about calling std::move on a local object in a return statement 
preventing copy elision.
+
 Wpmf-conversions
 C++ ObjC++ Var(warn_pmf2ptr) Init(1) Warning
 Warn when converting the type of pointers to member functions.
diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index 99be38ed8f8..122d9dcd4b3 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -9126,6 +9126,100 @@ maybe_warn_about_returning_address_of_local (tree 
retval)
   return false;
 }
 
+/* Returns true if DECL is in the std namespace.  */
+
+static bool
+decl_in_std_namespace_p (tree decl)
+{
+  return (decl != NULL_TREE
+ && DECL_NAMESPACE_STD_P (decl_namespace_context (decl)));
+}
+
+/* Returns true if FN, a CALL_EXPR, is a call to std::move.  */
+
+static bool
+is_std_move_p (tree fn)
+{
+  /* std::move only takes one argument.  */
+  if (call_expr_nargs (fn) != 1)
+return false;
+
+  tree fndecl = cp_get_callee_fndecl_nofold (fn);
+  if (!decl_in_std_namespace_p (fndecl))
+return false;
+
+  tree name = DECL_NAME (fndecl);
+  return name && id_equal (name, "move");
+}
+
+/* Returns true if RETVAL is a good candidate for the NRVO as per
+   [class.copy.elision].  FUNCTYPE is the type the function is declared
+   to return.  */
+
+static bool
+can_do_nrvo_p (tree retval, tree functype)
+{
+  tree result = DECL_RESULT (current_function_decl);
+  return (retval != NULL_TREE
+ && !processing_template_decl
+ /* Must be a local, automatic variable.  */
+ && VAR_P (retval)
+ && DECL_CONTEXT (retval) == current_function_decl
+ && !TREE_STATIC (retval)
+ /* And not a lambda or anonymous union proxy.  */
+ && !DECL_HAS_VALUE_EXPR_P (retval)
+ && (DECL_ALIGN (retval) <= DECL_ALIGN (result))
+ /* The cv-unqualified type of the returned value must be the
+same as the cv-unqualified return type of the
+function.  */
+ && same_type_p ((TYPE_MAIN_VARIANT (TREE_TYPE (retval))),
+ (TYPE_MAIN_VARIANT (functype)))
+ /* And the returned value must be non-volatile.  */
+ && !TYPE_VOLATILE (TREE_TYPE (retval)));
+}
+
+/* Warn about wrong usage of std::move in a return statement.  RETVAL
+   is the expression we are returning; FUNCTYPE is the type the function
+   is declared to return.  */
+
+static void
+maybe_warn_pessimizing_move (tree retval, tree functype)
+{
+  if (!warn_pessimizing_move)
+return;
+
+  /* C++98 doesn't know move.  */
+  if (cxx_dialect < cxx11)
+return;
+
+  /* This is only interesting for class types.  */
+  if (!CLASS_TYPE_P (functype))
+return;
+
+  /* We're looking for *std::move (&arg).  */
+  if (REFERENCE_REF_P (retval)
+  && TREE_CODE (TREE_OPERAND (retval, 0)) == CALL_EXPR)
+{
+  tree fn = TREE_OPERAND (retval, 0);
+  if (is_std_move_p (fn))
+   {
+ tree arg = CALL_EXPR_ARG (fn, 0);
+ STRIP_NOPS (arg);
+ if (TREE_CODE (arg) == ADDR_EXPR)
+   arg = TREE_OPERAND (arg, 0);
+ /* Warn if we could do copy elision were it not for the move.  */
+ if (can_do_nrvo_p (arg, functype))
+   {
+ auto_diagnostic_group d;
+ if (warning_at (location_of (retval), OPT_Wpessimizing_move,
+ "moving a local object in a return statement "
+ "prevents copy elision"))
+   inform (location_of (retval), "remove % call");
+   }
+   }
+}
+}
+
 /* Check that returning RETVAL from the current function is valid.
Return an expression explicitly showing all conversions require

[Ada] Crash processing SPARK annotate aspect

[Pierre-Marie de Rodat]

The compiler blows up writing the ALI file of a package that has a ghost
subprogram with an annotate contract.

Tested on x86_64-pc-linux-gnu, committed on trunk

2018-08-21  Javier Miranda  

gcc/ada/

* lib-writ.adb (Write_Unit_Information): Handle pragmas removed
by the expander.

gcc/testsuite/

* gnat.dg/spark2.adb, gnat.dg/spark2.ads: New testcase.--- gcc/ada/lib-writ.adb
+++ gcc/ada/lib-writ.adb
@@ -744,7 +744,14 @@ package body Lib.Writ is
   Note_Unit := U;
end if;
 
-   if Note_Unit = Unit_Num then
+   --  No action needed for pragmas removed by the expander (for
+   --  example, pragmas of ignored ghost entities).
+
+   if Nkind (N) = N_Null_Statement then
+  pragma Assert (Nkind (Original_Node (N)) = N_Pragma);
+  null;
+
+   elsif Note_Unit = Unit_Num then
   Write_Info_Initiate ('N');
   Write_Info_Char (' ');
 

--- /dev/null
new file mode 100644
+++ gcc/testsuite/gnat.dg/spark2.adb
@@ -0,0 +1,12 @@
+--  { dg-do compile }
+
+package body SPARK2 with SPARK_Mode is
+   function Factorial (N : Natural) return Natural is
+   begin
+  if N = 0 then
+ return 1;
+  else
+ return N * Factorial (N - 1);
+  end if;
+   end Factorial;
+end SPARK2;

--- /dev/null
new file mode 100644
+++ gcc/testsuite/gnat.dg/spark2.ads
@@ -0,0 +1,16 @@
+package SPARK2 with SPARK_Mode is
+
+   function Expon (Value, Exp : Natural) return Natural is
+  (if Exp = 0 then 1
+   else Value * Expon (Value, Exp - 1))
+   with Ghost,
+Pre => Value <= Max_Factorial_Number and Exp <= Max_Factorial_Number,
+Annotate => (GNATprove, Terminating);  --  CRASH!
+
+   Max_Factorial_Number : constant := 6;
+
+   function Factorial (N : Natural) return Natural with
+  Pre => N < Max_Factorial_Number,
+  Post => Factorial'Result <= Expon (Max_Factorial_Number, N);
+
+end SPARK2;

[Ada] Spurious error on overriding protected function in instance

[Pierre-Marie de Rodat]

The conformance between an overriding protected operation with
progenitors and the overridden interface operation requires subtype
conformance; requiring equality of return types in the case of a
function is too restrictive and leads to spurious errors when the return
type is a generic actual.

Tested on x86_64-pc-linux-gnu, committed on trunk

2018-08-21  Ed Schonberg  

gcc/ada/

* sem_ch6.adb (Check_Synchronized_Overriding): The conformance
between an overriding protected operation and the overridden
abstract progenitor operation requires subtype conformance;
requiring equality of return types in the case of a function is
too restrictive and leads to spurious errors when the return
type is a generic actual.

gcc/testsuite/

* gnat.dg/prot6.adb, gnat.dg/prot6.ads: New testcase.--- gcc/ada/sem_ch6.adb
+++ gcc/ada/sem_ch6.adb
@@ -7440,13 +7440,15 @@ package body Sem_Ch6 is
end;
 
 --  Functions can override abstract interface functions
+--  Return types must be subtype conformant.
 
 elsif Ekind (Def_Id) = E_Function
   and then Ekind (Subp) = E_Function
   and then Matches_Prefixed_View_Profile
  (Parameter_Specifications (Parent (Def_Id)),
   Parameter_Specifications (Parent (Subp)))
-  and then Etype (Def_Id) = Etype (Subp)
+  and then Conforming_Types (Etype (Def_Id), Etype (Subp),
+Subtype_Conformant)
 then
Candidate := Subp;
 

--- /dev/null
new file mode 100644
+++ gcc/testsuite/gnat.dg/prot6.adb
@@ -0,0 +1,20 @@
+--  { dg-do compile }
+--  { dg-options "-gnatc" }
+
+package body Prot6 is
+
+   protected body My_Type is
+
+  procedure Set (D : Integer) is
+  begin
+ I := D;
+  end Set;
+
+  function Get return Integer is
+  begin
+ return I;
+  end Get;
+   end My_Type;
+
+   procedure Dummy is null;
+end Prot6;

--- /dev/null
new file mode 100644
+++ gcc/testsuite/gnat.dg/prot6.ads
@@ -0,0 +1,31 @@
+package Prot6 is
+
+   generic
+  type TD is private;
+  type TI is synchronized interface;
+   package Set_Get is
+  type T is synchronized interface and TI;
+
+  procedure Set (E : in out T; D : TD) is abstract;
+  function Get (E : T) return TD is abstract;
+   end Set_Get;
+
+   type My_Type_Interface is synchronized interface;
+
+   package Set_Get_Integer is
+ new Set_Get (TD => Integer,
+  TI => My_Type_Interface);
+   use Set_Get_Integer;
+
+   protected type My_Type is
+new Set_Get_Integer.T with
+
+  overriding procedure Set (D : Integer);
+  overriding function Get return Integer;
+   private
+  I : Integer;
+   end My_Type;
+
+   procedure Dummy;
+
+end Prot6;

[Ada] Spurious ambiguity error on call returning an access type

[Pierre-Marie de Rodat]

If F is a function with a single defaulted parameter that returns an
access_to_array type, then F (I) may designate either the return type or
an indexing of the result of the call, after implicit dereferencing.  If
the component type C of the array type AR is accces AR this is ambiguous
in a context whose expected type is C. If F is parameterless the call is
not ambiguous.

Tested on x86_64-pc-linux-gnu, committed on trunk

2018-08-21  Ed Schonberg  

gcc/ada/

* sem_res.adb (Resolve_Call): Resolve correctly a parameterless
call that returns an access type whose designated type is the
component type of an array, when the function has no defaulted
parameters.

gcc/testsuite/

* gnat.dg/access5.adb, gnat.dg/access5.ads: New testcase.--- gcc/ada/sem_res.adb
+++ gcc/ada/sem_res.adb
@@ -6128,7 +6128,18 @@ package body Sem_Res is
 Ret_Type   : constant Entity_Id := Etype (Nam);
 
  begin
-if Is_Access_Type (Ret_Type)
+--  If this is a parameterless call there is no ambiguity
+--  and the call has the type of the function.
+
+if No (First_Actual (N)) then
+   Set_Etype (N, Etype (Nam));
+   if Present (First_Formal (Nam)) then
+  Resolve_Actuals (N, Nam);
+   end if;
+   Build_Call_Marker (N);
+
+elsif Is_Access_Type (Ret_Type)
+
   and then Ret_Type = Component_Type (Designated_Type (Ret_Type))
 then
Error_Msg_N

--- /dev/null
new file mode 100644
+++ gcc/testsuite/gnat.dg/access5.adb
@@ -0,0 +1,5 @@
+--  { dg-do compile }
+
+package body Access5 is
+  procedure Dummy is null;
+end;

--- /dev/null
new file mode 100644
+++ gcc/testsuite/gnat.dg/access5.ads
@@ -0,0 +1,10 @@
+package Access5 is
+  type Vec;
+  type Ptr is access all Vec;
+  type Vec is array (1..3) of Ptr;
+  function F return Ptr;
+  pragma Import (Ada, F);
+  Tail : Vec := (F, F, F);
+
+  procedure Dummy;
+end;

[Ada] Improper copying of limited arrays with default initialization

[Pierre-Marie de Rodat]

This patch fixes an improper expansion of aggregates for limited array
types in an object declaration. Prior to this patch, The presence of the
aggregate (which can only consist of box initializations) would create a
temporary that was then assigned to the object in the declaration.
Apart from a violation of the semantics of build-in-place limited
objects, this can also lead to out-of-scope access in LLVM.

Executing the following;

   gcc -c -gnatDG nocopy.adb
   grep quintet nocopy.adb.dg | wc -l

must yield:

   5


procedure NoCopy is

  --  Task used in this example to test that the limited component
  --  is properly initialized.

  task type T_Task (Disc : Natural);

  task body T_Task is
  begin
 null;
  end T_Task;

  type My_Rec (D : Natural := ) is record

 --  Components initialized by means of the current value
 --  of the record discriminant

 T : T_Task (D);
  end record;

  type TR is array (1 .. 5) of My_Rec;
  Quintet : TR := (others => (others => <>));
begin
   null;
end NoCopy;

Tested on x86_64-pc-linux-gnu, committed on trunk

2018-08-21  Ed Schonberg  

gcc/ada/

* exp_aggr.adb (Expand_Array_Aggregate): If the component type
is limited, the array must be constructed in place, so set flag
In_Place_Assign_OK_For_Declaration accordingly. This prevents
improper copying of an array of tasks during initialization.--- gcc/ada/exp_aggr.adb
+++ gcc/ada/exp_aggr.adb
@@ -6195,10 +6195,11 @@ package body Exp_Aggr is
   --  Look if in place aggregate expansion is possible
 
   --  For object declarations we build the aggregate in place, unless
-  --  the array is bit-packed or the component is controlled.
+  --  the array is bit-packed.
 
   --  For assignments we do the assignment in place if all the component
-  --  associations have compile-time known values. For other cases we
+  --  associations have compile-time known values, or are default-
+  --  initialized limited components, e.g. tasks. For other cases we
   --  create a temporary. The analysis for safety of on-line assignment
   --  is delicate, i.e. we don't know how to do it fully yet ???
 
@@ -6211,7 +6212,12 @@ package body Exp_Aggr is
  Establish_Transient_Scope (N, Manage_Sec_Stack => False);
   end if;
 
-  if Has_Default_Init_Comps (N) then
+  --  An array of limited components is built in place.
+
+  if Is_Limited_Type (Typ) then
+ Maybe_In_Place_OK := True;
+
+  elsif Has_Default_Init_Comps (N) then
  Maybe_In_Place_OK := False;
 
   elsif Is_Bit_Packed_Array (Typ)
@@ -6247,15 +6253,17 @@ package body Exp_Aggr is
   --  expected to appear in qualified form. In-place expansion eliminates
   --  the qualification and eventually violates this SPARK 05 restiction.
 
-  --  Should document the rest of the guards ???
+  --  Arrays of limited components must be built in place. The code
+  --  previously excluded controlled components but this is an old
+  --  oversight: the rules in 7.6 (17) are clear.
 
-  if not Has_Default_Init_Comps (N)
+  if (not Has_Default_Init_Comps (N)
+or else Is_Limited_Type (Etype (N)))
 and then Comes_From_Source (Parent_Node)
 and then Parent_Kind = N_Object_Declaration
 and then Present (Expression (Parent_Node))
 and then not
   Must_Slide (Etype (Defining_Identifier (Parent_Node)), Typ)
-and then not Has_Controlled_Component (Typ)
 and then not Is_Bit_Packed_Array (Typ)
 and then not Restriction_Check_Required (SPARK_05)
   then
@@ -6292,6 +6300,15 @@ package body Exp_Aggr is
  Set_Expansion_Delayed (N);
  return;
 
+  --  Limited arrays in return statements are expanded when
+  --  enclosing construct is expanded.
+
+  elsif Maybe_In_Place_OK
+and then Nkind (Parent (N)) = N_Simple_Return_Statement
+  then
+ Set_Expansion_Delayed (N);
+ return;
+
   --  In the remaining cases the aggregate is the RHS of an assignment
 
   elsif Maybe_In_Place_OK
@@ -6365,7 +6382,9 @@ package body Exp_Aggr is
 Target := New_Occurrence_Of (Tmp, Loc);
 
  else
-if Has_Default_Init_Comps (N) then
+if Has_Default_Init_Comps (N)
+  and then not Maybe_In_Place_OK
+then
 
--  Ada 2005 (AI-287): This case has not been analyzed???
 

[Ada] Crash on expression function and tagged types

[Pierre-Marie de Rodat]

This patch fixes a compiler abort on an expression function whose
expression includes tagged types that have not been frozen before the
generated body of the function is analyzed, even though that body is
inserted at the end of the current declarative part.

Tested on x86_64-pc-linux-gnu, committed on trunk

2018-08-21  Ed Schonberg  

gcc/ada/

* sem_ch6.adb (Analyze_Subprogram_Body_Helper, Mask_Type):
Refine the handling of freezing types for expression functions
that are not completions, when analyzing the generated body for
the function: the body is inserted at the end of the enclosing
declarative part, and its analysis may freeze types declared in
the same scope that have not been frozen yet.

gcc/testsuite/

* gnat.dg/expr_func7.adb, gnat.dg/expr_func7.ads: New testcase.--- gcc/ada/sem_ch6.adb
+++ gcc/ada/sem_ch6.adb
@@ -3145,8 +3145,12 @@ package body Sem_Ch6 is
end if;
 
if not Is_Frozen (Typ) then
-  Set_Is_Frozen (Typ);
-  Append_New_Elmt (Typ, Result);
+  if Scope (Typ) /= Current_Scope then
+ Set_Is_Frozen (Typ);
+ Append_New_Elmt (Typ, Result);
+  else
+ Freeze_Before (N, Typ);
+  end if;
end if;
 end Mask_Type;
 
@@ -3636,28 +3640,28 @@ package body Sem_Ch6 is
  --  They are necessary in any case to insure order of elaboration
  --  in gigi.
 
- if not Is_Frozen (Spec_Id)
+ if Nkind (N) = N_Subprogram_Body
+   and then Was_Expression_Function (N)
+   and then not Has_Completion (Spec_Id)
+   and then Serious_Errors_Detected = 0
and then (Expander_Active
   or else ASIS_Mode
-  or else (Operating_Mode = Check_Semantics
-and then Serious_Errors_Detected = 0))
+  or else Operating_Mode = Check_Semantics)
  then
 --  The body generated for an expression function that is not a
 --  completion is a freeze point neither for the profile nor for
 --  anything else. That's why, in order to prevent any freezing
 --  during analysis, we need to mask types declared outside the
---  expression that are not yet frozen.
+--  expression (and in an outer scope) that are not yet frozen.
 
-if Nkind (N) = N_Subprogram_Body
-  and then Was_Expression_Function (N)
-  and then not Has_Completion (Spec_Id)
-then
-   Set_Is_Frozen (Spec_Id);
-   Mask_Types := Mask_Unfrozen_Types (Spec_Id);
-else
-   Set_Has_Delayed_Freeze (Spec_Id);
-   Freeze_Before (N, Spec_Id);
-end if;
+Set_Is_Frozen (Spec_Id);
+Mask_Types := Mask_Unfrozen_Types (Spec_Id);
+
+ elsif not Is_Frozen (Spec_Id)
+   and then Serious_Errors_Detected = 0
+ then
+Set_Has_Delayed_Freeze (Spec_Id);
+Freeze_Before (N, Spec_Id);
  end if;
   end if;
 

--- /dev/null
new file mode 100644
+++ gcc/testsuite/gnat.dg/expr_func7.adb
@@ -0,0 +1,5 @@
+--  { dg-do compile }
+
+package body Expr_Func7 is
+   procedure Dummy is null;
+end Expr_Func7;

--- /dev/null
new file mode 100644
+++ gcc/testsuite/gnat.dg/expr_func7.ads
@@ -0,0 +1,20 @@
+package Expr_Func7 is
+
+   type Abstract_Food is tagged null record;
+   type Abstract_Food_Access is access Abstract_Food'Class;
+
+   type Fruit is new Abstract_Food with record
+  Worm : Boolean;
+   end record;
+
+   type Bananas is tagged record
+  Inside : Abstract_Food_Access;
+   end record;
+
+   function Has_Worm
+ (B : Bananas) return Boolean is (Fruit (B.Inside.all).Worm);
+
+   Cool : Bananas;
+
+   procedure Dummy;
+end Expr_Func7;

[Ada] Add a new gnat tool vxlink

[Pierre-Marie de Rodat]

VxLink is a helper tool used as a wrapper around g++/gcc to build
VxWorks DKM (Downloadable Kernel Modules).

Such DKM is a partially linked object that includes entry points for
constructors and destructors.

This tool thus uses g++ to generate an intermediate partially linked
object, retrieves the list of constructors and destructors in it and
produces a C file that lists those ctors/dtors in a way that is
understood be VxWorks kernel. It then links this file with the
intermediate object to produce a valid DKM.

Tested on x86_64-pc-linux-gnu, committed on trunk

2018-08-21  Jerome Lambourg  

gcc/ada/

* vxlink-bind.adb, vxlink-bind.ads, vxlink-link.adb,
vxlink-link.ads, vxlink-main.adb, vxlink.adb, vxlink.ads: Add a
new tool vxlink to handle VxWorks constructors in DKMs.
* gcc-interface/Makefile.in: add rules to build vxlink--- gcc/ada/gcc-interface/Makefile.in
+++ gcc/ada/gcc-interface/Makefile.in
@@ -441,6 +441,11 @@ ifeq ($(ENABLE_VXADDR2LINE),true)
 	  TOOLSCASE=cross top_buildir=../../.. \
 	  ../../vxaddr2line$(exeext)
 endif
+ifeq ($(ENABLE_VXLINK),true)
+	$(MAKE) -C tools -f ../Makefile $(TOOLS_FLAGS_TO_PASS) \
+	  TOOLSCASE=cross top_build=../../.. \
+	  ../../vxlink$(exeext)
+endif
 
 common-tools: ../stamp-tools
 	$(GNATMAKE) -j0 -c -b $(ADA_INCLUDES) \
@@ -478,6 +483,12 @@ common-tools: ../stamp-tools
 	$(GNATLINK) -v vxaddr2line -o $@ \
 	  --GCC="$(CC) $(ADA_INCLUDES)" --LINK="$(GCC_LINK)" ../targext.o $(CLIB)
 
+../../vxlink$(exeext): ../stamp-tools
+	$(GNATMAKE) -c  $(ADA_INCLUDES) vxlink-main --GCC="$(CC) $(ALL_ADAFLAGS)"
+	$(GNATBIND) $(ADA_INCLUDES) $(GNATBIND_FLAGS) vxlink-main
+	$(GNATLINK) -v vxlink-main -o $@ \
+	  --GCC="$(CC) $(ADA_INCLUDES)" --LINK="$(GCC_LINK)"
+
 gnatmake-re: ../stamp-tools
 	$(GNATMAKE) -j0 $(ADA_INCLUDES) -u sdefault --GCC="$(CC) $(MOST_ADA_FLAGS)"
 	$(GNATMAKE) -j0 -c $(ADA_INCLUDES) gnatmake --GCC="$(CC) $(ALL_ADAFLAGS)"

--- /dev/null
new file mode 100644
+++ gcc/ada/vxlink-bind.adb
@@ -0,0 +1,390 @@
+--
+--  --
+-- GNAT COMPILER COMPONENTS --
+--  --
+--  V X L I N K . B I N D   --
+--  --
+-- B o d y  --
+--  --
+-- Copyright (C) 2018, AdaCore  --
+--  --
+-- GNAT is free software;  you can  redistribute it  and/or modify it under --
+-- terms of the  GNU General Public License as published  by the Free Soft- --
+-- ware  Foundation;  either version 3,  or (at your option) any later ver- --
+-- sion.  GNAT is distributed in the hope that it will be useful, but WITH- --
+-- OUT 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  distributed with GNAT; see file COPYING3.  If not, go to --
+-- http://www.gnu.org/licenses for a complete copy of the license.  --
+--  --
+-- GNAT was originally developed  by the GNAT team at  New York University. --
+-- Extensive contributions were provided by Ada Core Technologies Inc.  --
+--  --
+--
+
+pragma Ada_2012;
+
+with Ada.Text_IO;   use Ada.Text_IO;
+with Ada.IO_Exceptions;
+with Ada.Strings.Fixed;
+
+with GNAT.Regpat;   use GNAT.Regpat;
+
+package body VxLink.Bind is
+
+   function Split_Lines (S : String) return Strings_List.Vector;
+
+   function Split (S : String; C : Character) return Strings_List.Vector;
+
+   function Parse_Nm_Output (S : String) return Symbol_Sets.Set;
+
+   procedure Emit_Module_Dtor
+ (FP : File_Type);
+
+   procedure Emit_CDtor
+ (FP  : File_Type;
+  Var : String;
+  Set : Symbol_Sets.Set);
+
+   -
+   -- Split_Lines --
+   -
+
+   function Split_Lines (S : String) return Strings_List.Vector
+   is
+  Last : Natural := S'First;
+  Ret  : Strings_List.Vector;
+   begin
+  for J in S'Range loop
+ if S (J) = ASCII.CR
+   and then J < S'Last
+   and then S (J + 1) = ASCII.LF
+ then
+Ret.Append (S (Last .. J - 1));
+Last := J + 2;
+ elsif S (J) = ASCII.LF then
+Ret.A

[Ada] Spurious crash on expression function as completion with contracts

[Pierre-Marie de Rodat]

This patch fixes a compiler abort on an expression function that is a
completion of a subprogram with preconditions. The problem is caused by
the presence of types in the precondition that are not frozen when the
subprogram body constructed for the expression function receives the
code that enforces the precondition. These types must be frozen before
the contract is expanded, so the freeze nodes for these types appear in
the proper scope. This is analogous to what is done with type references
that appear in the original expression of the expression function.

Tested on x86_64-pc-linux-gnu, committed on trunk

2018-08-21  Ed Schonberg  

gcc/ada/

* sem_ch6.adb: Remove Freeze_Expr_Types.
* freeze.ads, freeze.adb (Freeze_Expr_Types): Moved from
sem_ch6.adb, and extended to handle other expressions that may
contain unfrozen types that must be frozen in their proper
scopes.
* contracts.adb (Analyze_Entry_Or_Subprogram_Contract): If the
contract is for the generated body of an expression function
that is a completion, traverse the expressions for pre- and
postconditions to freeze all types before adding the contract
code within the subprogram body.

gcc/testsuite/

* gnat.dg/expr_func6.adb, gnat.dg/expr_func6.ads: New testcase.--- gcc/ada/contracts.adb
+++ gcc/ada/contracts.adb
@@ -32,6 +32,7 @@ with Errout;   use Errout;
 with Exp_Prag; use Exp_Prag;
 with Exp_Tss;  use Exp_Tss;
 with Exp_Util; use Exp_Util;
+with Freeze;   use Freeze;
 with Namet;use Namet;
 with Nlists;   use Nlists;
 with Nmake;use Nmake;
@@ -47,6 +48,7 @@ with Sem_Prag; use Sem_Prag;
 with Sem_Util; use Sem_Util;
 with Sinfo;use Sinfo;
 with Snames;   use Snames;
+with Stand;use Stand;
 with Stringt;  use Stringt;
 with SCIL_LL;  use SCIL_LL;
 with Tbuild;   use Tbuild;
@@ -589,14 +591,40 @@ package body Contracts is
  if Skip_Assert_Exprs then
 null;
 
- --  Otherwise analyze the pre/postconditions
+ --  Otherwise analyze the pre/postconditions. Their expressions
+ --  might include references to types that are not frozen yet,
+ --  in the case where the body is a rewritten expression function
+ --  that is a completion, so freeze all types within before
+ --  constructing the contract code.
 
  else
-Prag := Pre_Post_Conditions (Items);
-while Present (Prag) loop
-   Analyze_Pre_Post_Condition_In_Decl_Part (Prag, Freeze_Id);
-   Prag := Next_Pragma (Prag);
-end loop;
+declare
+   Bod : Node_Id;
+   Freeze_Types : Boolean := False;
+begin
+   if Present (Freeze_Id) then
+  Bod := Unit_Declaration_Node (Freeze_Id);
+  if Nkind (Bod) = N_Subprogram_Body
+and then Was_Expression_Function (Bod)
+and then Ekind (Subp_Id) = E_Function
+and then Chars (Subp_Id) = Chars (Freeze_Id)
+and then Subp_Id /= Freeze_Id
+  then
+ Freeze_Types := True;
+  end if;
+   end if;
+
+   Prag := Pre_Post_Conditions (Items);
+   while Present (Prag) loop
+  if Freeze_Types then
+ Freeze_Expr_Types (Subp_Id, Standard_Boolean,
+   Expression (Corresponding_Aspect (Prag)), Bod);
+  end if;
+
+  Analyze_Pre_Post_Condition_In_Decl_Part (Prag, Freeze_Id);
+  Prag := Next_Pragma (Prag);
+   end loop;
+end;
  end if;
 
  --  Analyze contract-cases and test-cases

--- gcc/ada/freeze.adb
+++ gcc/ada/freeze.adb
@@ -49,6 +49,7 @@ with Rtsfind;   use Rtsfind;
 with Sem;   use Sem;
 with Sem_Aux;   use Sem_Aux;
 with Sem_Cat;   use Sem_Cat;
+with Sem_Ch3;   use Sem_Ch3;
 with Sem_Ch6;   use Sem_Ch6;
 with Sem_Ch7;   use Sem_Ch7;
 with Sem_Ch8;   use Sem_Ch8;
@@ -7643,6 +7644,209 @@ package body Freeze is
   In_Spec_Expression := In_Spec_Exp;
end Freeze_Expression;
 
+   ---
+   -- Freeze_Expr_Types --
+   ---
+
+   procedure Freeze_Expr_Types
+ (Def_Id : Entity_Id;
+  Typ: Entity_Id;
+  Expr   : Node_Id;
+  N  : Node_Id)
+   is
+
+  function Cloned_Expression return Node_Id;
+  --  Build a duplicate of the expression of the return statement that
+  --  has no defining entities shared with the original expression.
+
+  function Freeze_Type_Refs (Node : Node_Id) return Traverse_Result;
+  --  Freeze all types referenced in the subtree rooted at Node
+
+  ---
+  -- Cloned_Expression --
+  ---
+
+  function Cloned_Expression return Node_Id is
+ function Clone_Id (Node : Node_Id) return Traver

[Ada] Retention of with clauses for ignored Ghost units

[Pierre-Marie de Rodat]

This patch ensures that with clauses that mention ignored Ghost units are
retained in the tree. The retention is necessary for several reasons:

   * The with clauses allow the new elaboration order mechanism to
 produce the same library edges regardless of whether the Ghost unit
 is checked or ignored. This ensures that the elaboration order
 remains consistent.

   * The with clauses allow the unnesting mechanism to properly
 recognize that all units have been examined for unnesing purposes.

No observable impact, no test needed.

Tested on x86_64-pc-linux-gnu, committed on trunk

2018-08-21  Hristian Kirtchev  

gcc/ada/

* sem_ch10.adb: Remove the with and use clause for unit Ghost.
(Analyze_With_Clause): Do not mark a with clause which mentions
an ignored Ghost code for elimination.--- gcc/ada/sem_ch10.adb
+++ gcc/ada/sem_ch10.adb
@@ -34,7 +34,6 @@ with Elists;use Elists;
 with Fname; use Fname;
 with Fname.UF;  use Fname.UF;
 with Freeze;use Freeze;
-with Ghost; use Ghost;
 with Impunit;   use Impunit;
 with Inline;use Inline;
 with Lib;   use Lib;
@@ -2912,8 +2911,6 @@ package body Sem_Ch10 is
Set_Fatal_Error (Current_Sem_Unit, Error_Ignored);
 end if;
   end case;
-
-  Mark_Ghost_Clause (N);
end Analyze_With_Clause;
 
--

[Ada] Fix internal error on extension of record with representation clause

[Pierre-Marie de Rodat]

This fixes a long-standing issue present for extensions of tagged record
types with a representation clause: the clause is correctly inherited
for components inherited in the extension but the position and size are
not, which fools the logic of Is_Possibly_Unaligned_Object.

This can result in an attempt to take the address of a component not
aligned on a byte boundary, which is then flagged as an internal error.

Tested on x86_64-pc-linux-gnu, committed on trunk

2018-08-21  Eric Botcazou  

gcc/ada/

* exp_util.adb (Is_Possibly_Unaligned_Object): For the case of a
selected component inherited in a record extension and subject
to a representation clause, retrieve the position and size from
the original record component.

gcc/testsuite/

* gnat.dg/rep_clause7.adb: New testcase.--- gcc/ada/exp_util.adb
+++ gcc/ada/exp_util.adb
@@ -8402,9 +8402,26 @@ package body Exp_Util is
 
declare
   Align_In_Bits : constant Nat := M * System_Storage_Unit;
+  Off : Uint;
+  Siz : Uint;
begin
-  if Component_Bit_Offset (C) mod Align_In_Bits /= 0
-or else Esize (C) mod Align_In_Bits /= 0
+  --  For a component inherited in a record extension, the
+  --  clause is inherited but position and size are not set.
+
+  if Is_Base_Type (Etype (P))
+and then Is_Tagged_Type (Etype (P))
+and then Present (Original_Record_Component (C))
+  then
+ Off :=
+   Component_Bit_Offset (Original_Record_Component (C));
+ Siz := Esize (Original_Record_Component (C));
+  else
+ Off := Component_Bit_Offset (C);
+ Siz := Esize (C);
+  end if;
+
+  if Off mod Align_In_Bits /= 0
+or else Siz mod Align_In_Bits /= 0
   then
  return True;
   end if;

--- /dev/null
new file mode 100644
+++ gcc/testsuite/gnat.dg/rep_clause7.adb
@@ -0,0 +1,29 @@
+procedure Rep_Clause7 is
+
+   subtype Msg is String (1 .. 3);
+
+   type Root is tagged record
+ B : Boolean;
+ M : Msg;
+   end record;
+   for Root use record
+ B at 0 range 64 .. 64;
+ M at 0 range 65 .. 88;
+   end record;
+
+   type Ext is new Root with null record;
+
+   procedure Inner (T : Msg) is
+   begin
+  null;
+   end;
+
+   pragma Warnings (Off);
+   T1 : Root;
+   T2 : Ext;
+   pragma Warnings (On);
+
+begin
+   Inner (T1.M);
+   Inner (T2.M);
+end;

[Ada] Define versions of dimension system for Float and Long_Float

[Pierre-Marie de Rodat]

The dimension system in System.Dim.Mks is based on Long_Long_Float,
which may not be convenient to people who want to use Float or
Long_Float as basis. These new files provide units that define the
dimension system based on Float in System.Dim.Float_Mks and on
Long_Float in System.Dim.Long_Mks.

Child packages Other_Prefixes and Mks_IO are also defined appropriately,
with new instantiations for the various sizes of floating-point types.

Tested on x86_64-pc-linux-gnu, committed on trunk

2018-08-21  Yannick Moy  

gcc/ada/

* doc/gnat_ugn/gnat_and_program_execution.rst: Update
documentation of dimensionality analysis.
* gnat_ugn.texi: Regenerate.
* Makefile.rtl, impunit.adb: Consider the new units.
* libgnat/s-dfmkio.ads, libgnat/s-dfmopr.ads,
libgnat/s-diflmk.ads: New units based on Float.
* libgnat/s-dilomk.ads, libgnat/s-dlmkio.ads,
libgnat/s-dlmopr.ads: New units based on Long_Float.
* libgnat/s-dmotpr.ads: Rename to libgnat/s-dgmgop.ads and turn
into an instance of
System.Dim.Generic_Mks.Generic_Other_Prefixes.
* libgnat/s-dimmks.ads: Rename to libgnat/s-digemk.ads and turn
into an instance of System.Dim.Generic_Mks.--- gcc/ada/Makefile.rtl
+++ gcc/ada/Makefile.rtl
@@ -523,12 +523,20 @@ GNATRTL_NONTASKING_OBJS= \
   s-conca9$(objext) \
   s-crc32$(objext)  \
   s-crtl$(objext)   \
+  s-dfmkio$(objext) \
+  s-dfmopr$(objext) \
+  s-dgmgop$(objext) \
+  s-dlmopr$(objext) \
   s-diflio$(objext) \
+  s-diflmk$(objext) \
+  s-digemk$(objext) \
   s-diinio$(objext) \
+  s-dilomk$(objext) \
   s-dim$(objext)\
   s-dimkio$(objext) \
   s-dimmks$(objext) \
   s-direio$(objext) \
+  s-dlmkio$(objext) \
   s-dmotpr$(objext) \
   s-dsaser$(objext) \
   s-elaall$(objext) \
@@ -2762,4 +2770,3 @@ a-tags.o  : a-tags.adb a-tags.ads
 s-memory.o  : s-memory.adb s-memory.ads
 	$(ADAC) -c $(ALL_ADAFLAGS) $(NO_SIBLING_ADAFLAGS) $(ADA_INCLUDES) \
 	  $< $(OUTPUT_OPTION)
-

--- gcc/ada/doc/gnat_ugn/gnat_and_program_execution.rst
+++ gcc/ada/doc/gnat_ugn/gnat_and_program_execution.rst
@@ -3280,19 +3280,18 @@ to use the proper subtypes in object declarations.
 .. index:: MKS_Type type
 
 The simplest way to impose dimensionality checking on a computation is to make
-use of the package ``System.Dim.Mks``,
-which is part of the GNAT library. This
-package defines a floating-point type ``MKS_Type``,
-for which a sequence of
-dimension names are specified, together with their conventional abbreviations.
-The following should be read together with the full specification of the
-package, in file :file:`s-dimmks.ads`.
+use of one of the instantiations of the package ``System.Dim.Generic_Mks``, which
+are part of the GNAT library. This generic package defines a floating-point
+type ``MKS_Type``, for which a sequence of dimension names are specified,
+together with their conventional abbreviations.  The following should be read
+together with the full specification of the package, in file
+:file:`s-digemk.ads`.
 
-  .. index:: s-dimmks.ads file
+  .. index:: s-digemk.ads file
 
   .. code-block:: ada
 
- type Mks_Type is new Long_Long_Float
+ type Mks_Type is new Float_Type
with
 Dimension_System => (
   (Unit_Name => Meter,Unit_Symbol => 'm',   Dim_Symbol => 'L'),
@@ -3336,10 +3335,16 @@ as well as useful multiples of these units:
  day : constant Time   := 60.0 * 24.0 * min;
 ...
 
-Using this package, you can then define a derived unit by
-providing the aspect that
-specifies its dimensions within the MKS system, as well as the string to
-be used for output of a value of that unit:
+There are three instantiations of ``System.Dim.Generic_Mks`` defined in the
+GNAT library:
+
+* ``System.Dim.Float_Mks`` based on ``Float`` defined in :file:`s-diflmk.ads`.
+* ``System.Dim.Long_Mks`` based on ``Long_Float`` defined in :file:`s-dilomk.ads`.
+* ``System.Dim.Mks`` based on ``Long_Long_Float`` defined in :file:`s-dimmks.ads`.
+
+Using one of these packages, you can then define a derived unit by providing
+the aspect that specifies its dimensions within the MKS system, as well as the
+string to be used for output of a value of that unit:
 
   .. code-block:: ada
 

--- gcc/ada/gnat_ugn.texi
+++ gcc/ada/gnat_ugn.texi
@@ -21,7 +21,7 @@
 
 @copying
 @quotation
-GNAT User's Guide for Native Platforms , Jul 13, 2018
+GNAT User's Guide for Native Platforms , Aug 17, 2018
 
 AdaCore
 
@@ -22606,20 +22606,19 @@ to use the proper subtypes in object declarations.
 @geindex MKS_Type type
 
 The simplest way to impose dimensionality checking on a computation is to make
-use of the package @code{System.Dim.Mks},
-which is part of the GNAT library. This
-package defines a floating-point type @code{MKS_Type},
-for which a sequence of
-dimension names are specified, together with their conventional abbreviations.
-The following should be read together with the full specification of the
-package, in file @code

[libiberty patch] pex-unix error behaviour

[Nathan Sidwell]

This is the second patch for pex-unix's child spawning.  Right now, if 
we manage to (v)fork, but the execvp fails, we use write to emit an 
error and then _exit.  The parent discovers the child failed when trying 
to communicate or wait for it.  This can be improved in 2 ways.


1) If we know we're using vfork, we can tell the parent directly via the 
current stack frame and suitable use of volatiles.  The parent blocks 
until the child exits (via _exit or successful execvp).  We can then 
look where the child will have written error information and use that. 
The error looks like:


class-1_a.C:3:15: error: failed execvp of mapper '|nope': No such file 
or directory

3 | export module One;
  |   ^~~
(this is coming from the module code)

2) Otherwise, if we're using fork we keep the old behaviour, but we can 
use stdio rather than write directly.

The error looks like:

cc1plus: error trying to exec 'nope': execvp: No such file or directory
class-1_a.C:3:15: error: unexpected close from mapper '|nope'
3 | export module One;
  |   ^~~
class-1_a.C:3:15: error: mapper '|nope' exit status 255

(the cc1plus error is that coming from pex-unix, the later diagnostic is 
when the parent discovers something's wrong trying to communicate).


I had to change the indentation more than expected, to avoid a maybe 
clobbered warning about the (non-volatile) bad_fn.  If you'd like that 
indentation altered separately, let me know.


booted and tested on x86_64-linux.  I manually tested both the fork and 
vfork configurations with a non-existent child executable as shown above.


ok?

nathan
--
Nathan Sidwell
2018-08-21  Nathan Sidwell  

	* pex-unix.c (VFORK_STRING): Replace this string with ...
	(VFORK_IS_FORK): ... this boolean.
	(pex_unix_exec_child): Communicate from child to parent when using
	vfork.  Use stdio for error when using fork.

Index: pex-unix.c
===
--- pex-unix.c	(revision 263701)
+++ pex-unix.c	(working copy)
@@ -60,9 +60,9 @@ extern int errno;
 #endif
 
 #ifdef vfork /* Autoconf may define this to fork for us. */
-# define VFORK_STRING "fork"
+# define VFORK_IS_FORK 1
 #else
-# define VFORK_STRING "vfork"
+# define VFORK_IS_FORK 0
 #endif
 #ifdef HAVE_VFORK_H
 #include 
@@ -579,10 +579,17 @@ pex_unix_exec_child (struct pex_obj *obj
  This clobbers the parent's environ so we need to restore it.
  It would be nice to use one of the exec* functions that takes an
  environment as a parameter, but that may have portability
- issues.   */
-  char **save_environ = environ;
-
-  const char *bad_fn = NULL;
+ issues.  It is marked volatile so the child doesn't consider it a
+ dead variable and therefore clobber where ever it is stored.  */
+  char **volatile save_environ = environ;
+
+  /* If we are using a true vfork, these allow the child to convey an
+ error to the parent immediately -- rather than discover it later
+ when trying to communicate.  Notice we're already in undefined
+ territory by not immediately calling execv[p] or _exit in the
+ child. */
+  volatile int child_errno = 0;
+  const char *volatile child_bad_fn = NULL;
 
   for (retries = 0; retries < 4; ++retries)
 {
@@ -595,113 +602,127 @@ pex_unix_exec_child (struct pex_obj *obj
 
   switch (pid)
 {
-case -1:
-  *err = errno;
-  *errmsg = VFORK_STRING;
-  return (pid_t) -1;
-
 case 0:
   /* Child process.  */
-  if (!bad_fn && in != STDIN_FILE_NO)
-	{
-	  if (dup2 (in, STDIN_FILE_NO) < 0)
-	bad_fn = "dup2";
-	  else if (close (in) < 0)
-	bad_fn = "close";
-	}
-  if (!bad_fn && out != STDOUT_FILE_NO)
-	{
-	  if (dup2 (out, STDOUT_FILE_NO) < 0)
-	bad_fn = "dup2";
-	  else if (close (out) < 0)
-	bad_fn = "close";
-	}
-  if (!bad_fn && errdes != STDERR_FILE_NO)
-	{
-	  if (dup2 (errdes, STDERR_FILE_NO) < 0)
-	bad_fn = "dup2";
-	  else if (close (errdes) < 0)
-	bad_fn = "close";
-	}
-  if (!bad_fn && toclose >= 0)
-	{
-	  if (close (toclose) < 0)
-	bad_fn = "close";
-	}
-  if (!bad_fn && (flags & PEX_STDERR_TO_STDOUT) != 0)
-	{
-	  if (dup2 (STDOUT_FILE_NO, STDERR_FILE_NO) < 0)
-	bad_fn = "dup2";
-	}
-  if (!bad_fn)
-	{
-	  if (env)
-	/* NOTE: In a standard vfork implementation this clobbers
-	   the parent's copy of environ "too" (in reality there's
-	   only one copy).  This is ok as we restore it below.  */
-	environ = (char**) env;
-	  if ((flags & PEX_SEARCH) != 0)
-	{
-	  execvp (executable, to_ptr32 (argv));
-	  bad_fn = "execvp";
-	}
-	  else
-	{
-	  execv (executable, to_ptr32 (argv));
-	  bad_fn = "execv";
-	}
-	}
-
-  /* Something failed, report an error.  We don't use stdio
-	 routines, because we might be here due to a vfork call.  */
   {
-	ssize_t retval = 0;
-	int err = errno;
+	const char *bad_fn = NULL;
+	if (!bad_fn && in != STDIN_FILE_NO)
+	  {
+	if (dup2 (in

[Ada] Crash on entry in generic with dynamic elaboration checks

[Pierre-Marie de Rodat]

This patch modifies the set of attributes that applies to entries and
entry families to include elaboration entities used by the
access-before-elaboration mechanism.

Tested on x86_64-pc-linux-gnu, committed on trunk

2018-08-21  Hristian Kirtchev  

gcc/ada/

* einfo.adb (Elaboration_Entity): Include entries and entry
families in the set of legal entities.
(Elaboration_Entity_Required): Include entries and entry
families in the set of legal entities.
(Set_Elaboration_Entity): Include entries and entry families in
the set of legal entities.
(Set_Elaboration_Entity_Required): Include entries and entry
families in the set of legal entities.
(Write_Field13_Name): Update the output of attribute
Elaboration_Entity.
* einfo.ads: Attributes Elaboration_Entity and
Elaboration_Entity_Required now apply to entries and entry
families.

gcc/testsuite/

* gnat.dg/elab6.adb, gnat.dg/elab6.ads, gnat.dg/elab6_pkg.adb,
gnat.dg/elab6_pkg.ads: New testcase.--- gcc/ada/einfo.adb
+++ gcc/ada/einfo.adb
@@ -1182,7 +1182,7 @@ package body Einfo is
   pragma Assert
 (Is_Subprogram (Id)
or else
- Ekind (Id) = E_Package
+ Ekind_In (Id, E_Entry, E_Entry_Family, E_Package)
or else
  Is_Generic_Unit (Id));
   return Node13 (Id);
@@ -1193,7 +1193,7 @@ package body Einfo is
   pragma Assert
 (Is_Subprogram (Id)
or else
- Ekind (Id) = E_Package
+ Ekind_In (Id, E_Entry, E_Entry_Family, E_Package)
or else
  Is_Generic_Unit (Id));
   return Flag174 (Id);
@@ -4412,7 +4412,7 @@ package body Einfo is
   pragma Assert
 (Is_Subprogram (Id)
or else
- Ekind (Id) = E_Package
+ Ekind_In (Id, E_Entry, E_Entry_Family, E_Package)
or else
  Is_Generic_Unit (Id));
   Set_Node13 (Id, V);
@@ -4423,7 +4423,7 @@ package body Einfo is
   pragma Assert
 (Is_Subprogram (Id)
or else
- Ekind (Id) = E_Package
+ Ekind_In (Id, E_Entry, E_Entry_Family, E_Package)
or else
  Is_Generic_Unit (Id));
   Set_Flag174 (Id, V);
@@ -10355,7 +10355,9 @@ package body Einfo is
  =>
 Write_Str ("Component_Clause");
 
- when E_Function
+ when E_Entry
+| E_Entry_Family
+| E_Function
 | E_Procedure
 | E_Package
 | Generic_Unit_Kind

--- gcc/ada/einfo.ads
+++ gcc/ada/einfo.ads
@@ -1090,10 +1090,10 @@ package Einfo is
 --   to the spec as possible.
 
 --Elaboration_Entity (Node13)
---   Defined in generic and non-generic package and subprogram entities.
---   This is a counter associated with the unit that is initially set to
---   zero, is incremented when an elaboration request for the unit is
---   made, and is decremented when a finalization request for the unit
+--   Defined in entry, entry family, [generic] package, and subprogram
+--   entities. This is a counter associated with the unit that is initially
+--   set to zero, is incremented when an elaboration request for the unit
+--   is made, and is decremented when a finalization request for the unit
 --   is made. This is used for three purposes. First, it is used to
 --   implement access before elaboration checks (the counter must be
 --   non-zero to call a subprogram at elaboration time). Second, it is
@@ -1110,9 +1110,9 @@ package Einfo is
 --   is elaboration code), but is simply not used for any purpose.
 
 --Elaboration_Entity_Required (Flag174)
---   Defined in generic and non-generic package and subprogram entities.
---   Set only if Elaboration_Entity is non-Empty to indicate that the
---   counter is required to be non-zero even if there is no other
+--   Defined in entry, entry family, [generic] package, and subprogram
+--   entities. Set only if Elaboration_Entity is non-Empty to indicate that
+--   the counter is required to be non-zero even if there is no other
 --   elaboration code. This occurs when the Elaboration_Entity counter
 --   is used for access before elaboration checks. If the counter is
 --   only used to prevent multiple execution of the elaboration code,
@@ -6058,6 +6058,7 @@ package Einfo is
--  E_Entry_Family
--Protected_Body_Subprogram   (Node11)
--Barrier_Function(Node12)
+   --Elaboration_Entity  (Node13)
--Postconditions_Proc (Node14)
--Entry_Parameters_Type   (Node15)
--First_Entity(Node17)

--- /dev/null
new file mode 100644
+++ gcc/testsuite/gnat.dg/elab6.adb
@@ -0,0 +1,8 @@
+--  { dg-do compile }
+--  { dg-options "-gnatE" }
+
+package body Elab6 is
+   procedure Force_Body is null;
+
+ 

[Ada] Spurious "Duplicated symbol" error with discriminated tasks

[Pierre-Marie de Rodat]

This patch fixes a spurious error in a program that contains a
discriminated task type and several of its subtype in the same
declarative part, when the corresponding discriminant constraints are
expressions.

Tested on x86_64-pc-linux-gnu, committed on trunk

2018-08-21  Ed Schonberg  

gcc/ada/

* sem_util.ads, sem_util.adb (New_External_Entity): Type of
Suffix_Index must be Int, not Nat, so that a negative value can
be used to generate a unique name for an external object, as
specified in Tbuild.New_External_Name.
(Scope_Within): Handle private type whose completion is a
synchronized type (For unnesting).
* itypes.ads, itypes.adb (Create_Itype): Ditto
* sem_ch3.adb (Constrain_Corresponding_Record): Generate a
unique name for the created subtype, because there may be
several discriminated tasks present in the same scope, and each
needs its distinct corresponding record subtype.

gcc/testsuite/

* gnat.dg/task1.adb, gnat.dg/task1.ads, gnat.dg/task1_pkg.adb,
gnat.dg/task1_pkg.ads: New testcase.--- gcc/ada/itypes.adb
+++ gcc/ada/itypes.adb
@@ -42,7 +42,7 @@ package body Itypes is
   Related_Nod  : Node_Id;
   Related_Id   : Entity_Id := Empty;
   Suffix   : Character := ' ';
-  Suffix_Index : Nat   := 0;
+  Suffix_Index : Int   := 0;
   Scope_Id : Entity_Id := Current_Scope) return Entity_Id
is
   Typ : Entity_Id;

--- gcc/ada/itypes.ads
+++ gcc/ada/itypes.ads
@@ -110,7 +110,7 @@ package Itypes is
   Related_Nod  : Node_Id;
   Related_Id   : Entity_Id := Empty;
   Suffix   : Character := ' ';
-  Suffix_Index : Nat   := 0;
+  Suffix_Index : Int   := 0;
   Scope_Id : Entity_Id := Current_Scope) return Entity_Id;
--  Used to create a new Itype
--

--- gcc/ada/sem_ch3.adb
+++ gcc/ada/sem_ch3.adb
@@ -9453,6 +9453,7 @@ package body Sem_Ch3 is
   (Derived_Type, Save_Discr_Constr);
 Set_Stored_Constraint
   (Derived_Type, Expand_To_Stored_Constraint (Parent_Type, Discs));
+
 Replace_Components (Derived_Type, New_Decl);
  end if;
 
@@ -13692,7 +13693,8 @@ package body Sem_Ch3 is
   Related_Nod : Node_Id) return Entity_Id
is
   T_Sub : constant Entity_Id :=
-Create_Itype (E_Record_Subtype, Related_Nod, Corr_Rec, 'C');
+Create_Itype (E_Record_Subtype,
+  Related_Nod, Corr_Rec, 'C', Suffix_Index => -1);
 
begin
   Set_Etype (T_Sub, Corr_Rec);

--- gcc/ada/sem_util.adb
+++ gcc/ada/sem_util.adb
@@ -20997,7 +20997,7 @@ package body Sem_Util is
   Sloc_Value   : Source_Ptr;
   Related_Id   : Entity_Id;
   Suffix   : Character;
-  Suffix_Index : Nat := 0;
+  Suffix_Index : Int := 0;
   Prefix   : Character := ' ') return Entity_Id
is
   N : constant Entity_Id :=
@@ -24039,6 +24039,15 @@ package body Sem_Util is
and then Outer = Protected_Body_Subprogram (Curr)
  then
 return True;
+
+ --  OUtside of its scope, a synchronized type may just be
+ --  private.
+
+ elsif Is_Private_Type (Curr)
+   and then Present (Full_View (Curr))
+and then Is_Concurrent_Type (Full_View (Curr))
+ then
+return Scope_Within (Full_View (Curr), Outer);
  end if;
   end loop;
 

--- gcc/ada/sem_util.ads
+++ gcc/ada/sem_util.ads
@@ -2326,7 +2326,7 @@ package Sem_Util is
   Sloc_Value   : Source_Ptr;
   Related_Id   : Entity_Id;
   Suffix   : Character;
-  Suffix_Index : Nat := 0;
+  Suffix_Index : Int := 0;
   Prefix   : Character := ' ') return Entity_Id;
--  This function creates an N_Defining_Identifier node for an internal
--  created entity, such as an implicit type or subtype, or a record

--- /dev/null
new file mode 100644
+++ gcc/testsuite/gnat.dg/task1.adb
@@ -0,0 +1,5 @@
+--  { dg-do assemble }
+
+package body Task1 is
+   procedure Dummy is null;
+end Task1;

--- /dev/null
new file mode 100644
+++ gcc/testsuite/gnat.dg/task1.ads
@@ -0,0 +1,10 @@
+with Task1_Pkg; use Task1_Pkg;
+
+package Task1 is
+   TAB : constant Typ_Task_Par_Tab := (others => (Dummy => FALSE));
+
+   T1 : Typ_Task (TAB (1).Dummy);
+   T2 : Typ_Task (TAB (2).Dummy);
+
+   procedure Dummy;
+end Task1;

--- /dev/null
new file mode 100644
+++ gcc/testsuite/gnat.dg/task1_pkg.adb
@@ -0,0 +1,11 @@
+package body Task1_Pkg is
+   task body Typ_Task is
+   begin
+  loop
+ null;
+  end loop;
+   end Typ_Task;
+
+begin
+   null;
+end Task1_Pkg;

--- /dev/null
new file mode 100644
+++ gcc/testsuite/gnat.dg/task1_pkg.ads
@@ -0,0 +1,10 @@
+package Task1_Pkg is
+   subtype Typ_Bool is boolean;
+
+   type Typ_Task_Par is record
+  Dummy : Typ_Bool;
+   end record;
+
+   type Typ_Task_Par_Tab is array (1 .. 33) of aliased Typ_Task_Par;
+   task type Typ

[Ada] Enumeration types with non-standard representation

[Pierre-Marie de Rodat]

The compiler may report errors on enumeration types with non-standard
representation (i.e. at least one literal has a representation value
different from its 'Pos value) processing attribute 'Enum_Rep.

It may also generate wrong code for the evaluation of 'Enum_Rep raising
Constraint_Error at runtime.

Tested on x86_64-pc-linux-gnu, committed on trunk

2018-08-21  Javier Miranda  

gcc/ada/

* checks.ads (Determine_Range): Adding documentation.
* checks.adb (Determine_Range): Don't deal with enumerated types
with non-standard representation.
(Convert_And_Check_Range): For conversion of enumeration types
with non standard representation to an integer type perform a
direct conversion to the target integer type.

gcc/testsuite/

* gnat.dg/enum4.adb: New testcase.--- gcc/ada/checks.adb
+++ gcc/ada/checks.adb
@@ -4490,6 +4490,11 @@ package body Checks is
 
 or else not Is_Discrete_Type (Typ)
 
+--  Don't deal with enumerated types with non-standard representation
+
+or else (Is_Enumeration_Type (Typ)
+   and then Present (Enum_Pos_To_Rep (Base_Type (Typ
+
 --  Ignore type for which an error has been posted, since range in
 --  this case may well be a bogosity deriving from the error. Also
 --  ignore if error posted on the reference node.
@@ -6758,9 +6763,36 @@ package body Checks is
   -
 
   procedure Convert_And_Check_Range is
- Tnn : constant Entity_Id := Make_Temporary (Loc, 'T', N);
+ Tnn   : constant Entity_Id := Make_Temporary (Loc, 'T', N);
+ Conv_Node : Node_Id;
 
   begin
+ --  For enumeration types with non-standard representation this is a
+ --  direct conversion from the enumeration type to the target integer
+ --  type, which is treated by the back end as a normal integer type
+ --  conversion, treating the enumeration type as an integer, which is
+ --  exactly what we want. We set Conversion_OK to make sure that the
+ --  analyzer does not complain about what otherwise might be an
+ --  illegal conversion.
+
+ if Is_Enumeration_Type (Source_Base_Type)
+   and then Present (Enum_Pos_To_Rep (Source_Base_Type))
+   and then Is_Integer_Type (Target_Base_Type)
+ then
+Conv_Node :=
+  OK_Convert_To (
+Typ  => Target_Base_Type,
+Expr => Duplicate_Subexpr (N));
+
+ --  Common case
+
+ else
+Conv_Node :=
+  Make_Type_Conversion (Loc,
+Subtype_Mark => New_Occurrence_Of (Target_Base_Type, Loc),
+Expression   => Duplicate_Subexpr (N));
+ end if;
+
  --  We make a temporary to hold the value of the converted value
  --  (converted to the base type), and then do the test against this
  --  temporary. The conversion itself is replaced by an occurrence of
@@ -6776,10 +6808,7 @@ package body Checks is
  Defining_Identifier => Tnn,
  Object_Definition   => New_Occurrence_Of (Target_Base_Type, Loc),
  Constant_Present=> True,
- Expression  =>
-   Make_Type_Conversion (Loc,
- Subtype_Mark => New_Occurrence_Of (Target_Base_Type, Loc),
- Expression   => Duplicate_Subexpr (N))),
+ Expression  => Conv_Node),
 
Make_Raise_Constraint_Error (Loc,
  Condition =>

--- gcc/ada/checks.ads
+++ gcc/ada/checks.ads
@@ -310,14 +310,16 @@ package Checks is
--  then OK is True on return, and Lo and Hi are set to a conservative
--  estimate of the possible range of values of N. Thus if OK is True on
--  return, the value of the subexpression N is known to lie in the range
-   --  Lo .. Hi (inclusive). If the expression is not of a discrete type, or
-   --  some kind of error condition is detected, then OK is False on exit, and
-   --  Lo/Hi are set to No_Uint. Thus the significance of OK being False on
-   --  return is that no useful information is available on the range of the
-   --  expression. Assume_Valid determines whether the processing is allowed to
-   --  assume that values are in range of their subtypes. If it is set to True,
-   --  then this assumption is valid, if False, then processing is done using
-   --  base types to allow invalid values.
+   --  Lo .. Hi (inclusive). For enumeration and character literals the values
+   --  returned are the Pos value in the relevant enumeration type. If the
+   --  expression is not of a discrete type, or some kind of error condition
+   --  is detected, then OK is False on exit, and Lo/Hi are set to No_Uint.
+   --  Thus the significance of OK being False on return is that no useful
+   --  information is available on the range of the expression. Assume_Valid
+   --  determines whether the

[Ada] Compiler abort on call to expr. function for default discriminant

[Pierre-Marie de Rodat]

If a discriminant specification has a default that is a call to an
expression function, that function has to be frozen at the point of a
call to the initialization procedure for an object of the record type,
even though the call does not appear to come from source.

Tested on x86_64-pc-linux-gnu, committed on trunk

2018-08-21  Ed Schonberg  

gcc/ada/

* sem_res.adb (Resolve_Call): Force the freezing of an
expression function that is called to provide a default value
for a defaulted discriminant in an object initialization.

gcc/testsuite/

* gnat.dg/expr_func5.adb: New testcase.--- gcc/ada/sem_res.adb
+++ gcc/ada/sem_res.adb
@@ -6067,7 +6067,10 @@ package body Sem_Res is
   --  (including the body of another expression function) which would
   --  place the freeze node in the wrong scope. An expression function
   --  is frozen in the usual fashion, by the appearance of a real body,
-  --  or at the end of a declarative part.
+  --  or at the end of a declarative part. However an implcit call to
+  --  an expression function may appear when it is part of a default
+  --  expression in a call to an initialiation procedure, and must be
+  --  frozen now, even if the body is inserted at a later point.
 
   if Is_Entity_Name (Subp)
 and then not In_Spec_Expression
@@ -6076,6 +6079,14 @@ package body Sem_Res is
   (not Is_Expression_Function_Or_Completion (Entity (Subp))
 or else Scope (Entity (Subp)) = Current_Scope)
   then
+ if Is_Expression_Function (Entity (Subp)) then
+
+--  Force freeze of expression function in call.
+
+Set_Comes_From_Source (Subp, True);
+Set_Must_Not_Freeze (Subp, False);
+ end if;
+
  Freeze_Expression (Subp);
   end if;
 

--- /dev/null
new file mode 100644
+++ gcc/testsuite/gnat.dg/expr_func5.adb
@@ -0,0 +1,10 @@
+--  { dg-do compile }
+
+procedure Expr_Func5 is
+   type T is (B);
+   function F return T is (B);
+   type R (W : T := F) is null record;
+   V : R;
+begin
+   null;
+end Expr_Func5;

[Ada] General purpose doubly linked list for compiler and tool use

[Pierre-Marie de Rodat]

This patch adds unit GNAT.Lists which currently contains the
implementation of a general purpose doubly linked list intended for use
by the compiler and the tools around it.

Tested on x86_64-pc-linux-gnu, committed on trunk

2018-08-21  Hristian Kirtchev  

gcc/ada/

* impunit.adb: Add g-lists to the set of non-implementation
units.
* libgnat/g-lists.adb, libgnat/g-lists.ads: New unit.
* Makefile.rtl: Add g-lists to the set of non-tasking units.
* gcc-interface/Make-lang.in: Add g-lists to the set of files
used by gnat1.

gcc/testsuite/

* gnat.dg/linkedlist.adb: New testcase.--- gcc/ada/Makefile.rtl
+++ gcc/ada/Makefile.rtl
@@ -427,6 +427,7 @@ GNATRTL_NONTASKING_OBJS= \
   g-htable$(objext) \
   g-io$(objext) \
   g-io_aux$(objext) \
+  g-lists$(objext) \
   g-locfil$(objext) \
   g-mbdira$(objext) \
   g-mbflra$(objext) \

--- gcc/ada/gcc-interface/Make-lang.in
+++ gcc/ada/gcc-interface/Make-lang.in
@@ -319,6 +319,7 @@ GNAT_ADA_OBJS =	\
  ada/libgnat/g-dynhta.o	\
  ada/libgnat/g-hesora.o	\
  ada/libgnat/g-htable.o	\
+ ada/libgnat/g-lists.o \
  ada/libgnat/g-spchge.o	\
  ada/libgnat/g-speche.o	\
  ada/libgnat/g-u3spch.o	\

--- gcc/ada/impunit.adb
+++ gcc/ada/impunit.adb
@@ -281,6 +281,7 @@ package body Impunit is
 ("g-htable", F),  -- GNAT.Htable
 ("g-io", F),  -- GNAT.IO
 ("g-io_aux", F),  -- GNAT.IO_Aux
+("g-lists ", F),  -- GNAT.Lists
 ("g-locfil", F),  -- GNAT.Lock_Files
 ("g-mbdira", F),  -- GNAT.MBBS_Discrete_Random
 ("g-mbflra", F),  -- GNAT.MBBS_Float_Random

--- /dev/null
new file mode 100644
+++ gcc/ada/libgnat/g-lists.adb
@@ -0,0 +1,635 @@
+--
+--  --
+-- GNAT RUN-TIME COMPONENTS --
+--  --
+--G N A T . L I S T S   --
+--  --
+-- B o d y  --
+--  --
+-- Copyright (C) 2018, Free Software Foundation, Inc.   --
+--  --
+-- GNAT is free software;  you can  redistribute it  and/or modify it under --
+-- terms of the  GNU General Public License as published  by the Free Soft- --
+-- ware  Foundation;  either version 3,  or (at your option) any later ver- --
+-- sion.  GNAT is distributed in the hope that it will be useful, but WITH- --
+-- OUT ANY WARRANTY;  without even the  implied warranty of MERCHANTABILITY --
+-- or FITNESS FOR A PARTICULAR PURPOSE. --
+--  --
+-- As a special exception under Section 7 of GPL version 3, you are granted --
+-- additional permissions described in the GCC Runtime Library Exception,   --
+-- version 3.1, as published by the Free Software Foundation.   --
+--  --
+-- You should have received a copy of the GNU General Public License and--
+-- a copy of the GCC Runtime Library Exception along with this program; --
+-- see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see--
+-- .  --
+--  --
+-- GNAT was originally developed  by the GNAT team at  New York University. --
+-- Extensive contributions were provided by Ada Core Technologies Inc.  --
+--  --
+--
+
+with Ada.Unchecked_Deallocation;
+
+package body GNAT.Lists is
+
+   package body Doubly_Linked_List is
+  procedure Delete_Node (L : Instance; Nod : Node_Ptr);
+  pragma Inline (Delete_Node);
+  --  Detach and delete node Nod from list L
+
+  procedure Ensure_Circular (Head : Node_Ptr);
+  pragma Inline (Ensure_Circular);
+  --  Ensure that dummy head Head is circular with respect to itself
+
+  procedure Ensure_Created (L : Instance);
+  pragma Inline (Ensure_Created);
+  --  Verify that list L is created. Raise Not_Created if this is not the
+  --  case.
+
+  procedure Ensure_Full (L : Instance);
+  pragma Inline (Ensure_Full);
+  --  Verify that list L contains at least one element. Raise List_Empty if
+  --  this is not the case.
+
+  procedure Ensure_Unlocked (L : Instance);
+  pragma Inline (Ensure_Unlocked);
+  --  Verify that list L is unlo

[Ada] Handle pragmas that come from aspects for GNATprove

[Pierre-Marie de Rodat]

In GNATprove we appear to abuse a routine related to cross-references and
expect it to deliver exact results, which is not what it was designed for.

This patch is a temporary solution to avoid crashes in GNATprove; it doesn't
affect the frontend or other backends, because this code is used exclusively
by GNATprove (it is located in the frontend for technical reasons). No tests
provided.

Tested on x86_64-pc-linux-gnu, committed on trunk

2018-08-21  Piotr Trojanek  

gcc/ada/

* lib-xref.ads, lib-xref-spark_specific.adb
(Enclosing_Subprogram_Or_Library_Package): Now roughtly works
for pragmas that come from aspect specifications.--- gcc/ada/lib-xref-spark_specific.adb
+++ gcc/ada/lib-xref-spark_specific.adb
@@ -228,7 +228,17 @@ package body SPARK_Specific is
end loop;
 
if Nkind (Context) = N_Pragma then
-  Context := Parent (Context);
+  --  When used for cross-references then aspects might not be
+  --  yet linked to pragmas; when used for AST navigation in
+  --  GNATprove this routine is expected to follow those links.
+
+  if From_Aspect_Specification (Context) then
+ Context := Corresponding_Aspect (Context);
+ pragma Assert (Nkind (Context) = N_Aspect_Specification);
+ Context := Entity (Context);
+  else
+ Context := Parent (Context);
+  end if;
end if;
 
 when N_Entry_Body

--- gcc/ada/lib-xref.ads
+++ gcc/ada/lib-xref.ads
@@ -632,6 +632,11 @@ package Lib.Xref is
   --  Return the closest enclosing subprogram or library-level package.
   --  This ensures that GNATprove can distinguish local variables from
   --  global variables.
+  --
+  --  ??? This routine should only be used for processing related to
+  --  cross-references, where it might return wrong result but must avoid
+  --  crashes on ill-formed source code. It is wrong to use it where exact
+  --  result is needed.
 
   procedure Generate_Dereference
 (N   : Node_Id;

[Ada] Dynamically resizable, load factor-based hash table

[Pierre-Marie de Rodat]

This patch introduces a dynamically resizable, load factor-based hash
table in unit GNAT.Dynamic_HTables.

Tested on x86_64-pc-linux-gnu, committed on trunk

2018-08-21  Hristian Kirtchev  

gcc/ada/

* libgnat/g-dynhta.adb, libgnat/g-dynhta.ads: New package
Dynamic_HTable.

gcc/testsuite/

* gnat.dg/dynhash.adb: New testcase.--- gcc/ada/libgnat/g-dynhta.adb
+++ gcc/ada/libgnat/g-dynhta.adb
@@ -38,11 +38,10 @@ package body GNAT.Dynamic_HTables is
---
 
package body Static_HTable is
-
   function Get_Non_Null (T : Instance) return Elmt_Ptr;
   --  Returns Null_Ptr if Iterator_Started is False or if the Table is
-  --  empty. Returns Iterator_Ptr if non null, or the next non null
-  --  element in table if any.
+  --  empty. Returns Iterator_Ptr if non null, or the next non null element
+  --  in table if any.
 
   -
   -- Get --
@@ -363,7 +362,834 @@ package body GNAT.Dynamic_HTables is
   begin
  E.Next := Next;
   end Set_Next;
-
end Simple_HTable;
 
+   
+   -- Dynamic_HTable --
+   
+
+   package body Dynamic_HTable is
+  Minimum_Size : constant Bucket_Range_Type := 32;
+  --  Minimum size of the buckets
+
+  Safe_Compression_Size : constant Bucket_Range_Type :=
+Minimum_Size * Compression_Factor;
+  --  Maximum safe size for hash table compression. Beyond this size, a
+  --  compression will violate the minimum size constraint on the buckets.
+
+  Safe_Expansion_Size : constant Bucket_Range_Type :=
+  Bucket_Range_Type'Last / Expansion_Factor;
+  --  Maximum safe size for hash table expansion. Beyond this size, an
+  --  expansion will overflow the buckets.
+
+  procedure Destroy_Buckets (Bkts : Bucket_Table_Ptr);
+  pragma Inline (Destroy_Buckets);
+  --  Destroy all nodes within buckets Bkts
+
+  procedure Detach (Nod : Node_Ptr);
+  pragma Inline (Detach);
+  --  Detach node Nod from the bucket it resides in
+
+  procedure Ensure_Circular (Head : Node_Ptr);
+  pragma Inline (Ensure_Circular);
+  --  Ensure that dummy head Head is circular with respect to itself
+
+  procedure Ensure_Created (T : Instance);
+  pragma Inline (Ensure_Created);
+  --  Verify that hash table T is created. Raise Not_Created if this is not
+  --  the case.
+
+  procedure Ensure_Unlocked (T : Instance);
+  pragma Inline (Ensure_Unlocked);
+  --  Verify that hash table T is unlocked. Raise Table_Locked if this is
+  --  not the case.
+
+  function Find_Bucket
+(Bkts : Bucket_Table_Ptr;
+ Key  : Key_Type) return Node_Ptr;
+  pragma Inline (Find_Bucket);
+  --  Find the bucket among buckets Bkts which corresponds to key Key, and
+  --  return its dummy head.
+
+  function Find_Node (Head : Node_Ptr; Key : Key_Type) return Node_Ptr;
+  pragma Inline (Find_Node);
+  --  Traverse a bucket indicated by dummy head Head to determine whether
+  --  there exists a node with key Key. If such a node exists, return it,
+  --  otherwise return null.
+
+  procedure First_Valid_Node
+(T: Instance;
+ Low_Bkt  : Bucket_Range_Type;
+ High_Bkt : Bucket_Range_Type;
+ Idx  : out Bucket_Range_Type;
+ Nod  : out Node_Ptr);
+  pragma Inline (First_Valid_Node);
+  --  Find the first valid node in the buckets of hash table T constrained
+  --  by the range Low_Bkt .. High_Bkt. If such a node exists, return its
+  --  bucket index in Idx and reference in Nod. If no such node exists,
+  --  Idx is set to 0 and Nod to null.
+
+  procedure Free is
+new Ada.Unchecked_Deallocation (Bucket_Table, Bucket_Table_Ptr);
+
+  procedure Free is
+new Ada.Unchecked_Deallocation (Hash_Table, Instance);
+
+  procedure Free is
+new Ada.Unchecked_Deallocation (Node, Node_Ptr);
+
+  function Is_Valid (Iter : Iterator) return Boolean;
+  pragma Inline (Is_Valid);
+  --  Determine whether iterator Iter refers to a valid key-value pair
+
+  function Is_Valid (Nod : Node_Ptr; Head : Node_Ptr) return Boolean;
+  pragma Inline (Is_Valid);
+  --  Determine whether node Nod is non-null and does not refer to dummy
+  --  head Head, thus making it valid.
+
+  function Load_Factor (T : Instance) return Threshold_Type;
+  pragma Inline (Load_Factor);
+  --  Calculate the load factor of hash table T
+
+  procedure Lock (T : Instance);
+  pragma Inline (Lock);
+  --  Lock all mutation functionality of hash table T
+
+  procedure Mutate_And_Rehash (T : Instance; Size : Bucket_Range_Type);
+  pragma Inline (Mutate_And_Rehash);
+  --  Replace the buckets of hash table T with a new set of buckets of size
+  --  Size. Rehash all key-value pairs from the old

Re: patch to bug #86829

[Jeff Law]

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


> 
> If we had useful range info on floats we might conditionalize such
> transforms appropriately.  Or we can enable it on floats and do
> the sqrt (x*x + 1) in double.
Yea.  I keep thinking about what it might take to start doing some light
VRP of floating point objects.  I'd originally been thinking to just
track 0.0 and exceptional value state.  But the more I ponder the more I
think we could use the range information to allow transformations that
are currently guarded by the -ffast-math family of options.

jeff

[PATCH, testsuite] Stringify __USER_LABEL_PREFIX__ in pr85248.

[Iain Sandoe]

another missing stringify for _U_L_P_ 

OK?
thanks
Iain

gcc/testsuite

* gcc.dg/lto/pr85248_0.c (test_alias): Stringify __USER_LABEL_PREFIX__.
(test_noreturn): Likewise.

---
 gcc/testsuite/gcc.dg/lto/pr85248_0.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/gcc/testsuite/gcc.dg/lto/pr85248_0.c 
b/gcc/testsuite/gcc.dg/lto/pr85248_0.c
index df61ac976a..902f4b6620 100644
--- a/gcc/testsuite/gcc.dg/lto/pr85248_0.c
+++ b/gcc/testsuite/gcc.dg/lto/pr85248_0.c
@@ -2,8 +2,13 @@
 /* { dg-lto-do run } */
 /* { dg-lto-options { { -flto -O2 } } } */
 
-extern void test_alias (int s, int e) __asm__ (__USER_LABEL_PREFIX__ "test");
-extern void test_noreturn (int s, int e) __asm__ (__USER_LABEL_PREFIX__ "test")
+#define STR1(X) #X
+#define STR2(X) STR1(X)
+
+extern void test_alias (int s, int e) 
+  __asm__ (STR2(__USER_LABEL_PREFIX__) "test");
+extern void test_noreturn (int s, int e)
+  __asm__ (STR2(__USER_LABEL_PREFIX__)  "test")
   __attribute__ ((__noreturn__));
 
 extern inline __attribute__ ((__always_inline__, __gnu_inline__)) void
-- 
2.17.1

Re: VRP: rewrite the division code (to handle corner cases including 0)

[Jeff Law]

On 08/21/2018 03:46 AM, Richard Biener wrote:
> 
> +  /* If we're definitely dividing by zero, there's nothing to do.  */
> +  if (wide_int_range_zero_p (divisor_min, divisor_max, prec))
> +return false;
> 
> I know we didn't do this before but for x / 0 we should compute UNDEFINED
> as range.  With the current interfacing this special case would require 
> handling
> in the non-wide-int caller.
No strong opinions here, just a couple notes.

If we set the result for this to VR_UNDEFINED, then it'll naturally be
handled optimistically at PHI nodes, COND_EXPRs, etc.

We'd normally get that effect via path isolation when we turn the x / 0
into a trap and simplify the CFG appropriately.

However, using VR_UNDEFINED for division by zero in VRP captures the
effect earlier which is generally a good thing.

Jeff

Re: C++ PATCH for c++/86981, implement -Wpessimizing-move

[David Malcolm]

On Tue, 2018-08-21 at 10:01 -0400, Marek Polacek wrote:
> On Mon, Aug 20, 2018 at 07:42:10PM -0400, David Malcolm wrote:
> > On Mon, 2018-08-20 at 18:54 -0400, Marek Polacek wrote:
> > > On Mon, Aug 20, 2018 at 05:18:49PM -0400, David Malcolm wrote:
> > > > As of r263675 it's now possible to tell the diagnostics
> > > > subsystem
> > > > that
> > > > the warning and note are related by using an
> > > > auto_diagnostic_group
> > > > instance [1], so please can this read:
> > > > 
> > > >   if (can_do_nrvo_p (arg, functype))
> > > > {
> > > >   auto_diagnostic_group d;
> > > >   if (warning (OPT_Wpessimizing_move, "moving a
> > > > local
> > > > object "
> > > >"in a return statement prevents copy
> > > > elision"))
> > > > inform (input_location, "remove %
> > > > call");
> > > > }
> > > 
> > > Sure:
> > > 
> > > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> > 
> > Thanks!  The changed part looks good, but I can't really speak to
> > the
> > rest of the patch.
> > 
> > (BTW, could we emit a fix-it hint as part of that "inform"
> > call?  How
> > good is the location information at this point?)
> 
> Thanks, I can actually use location_of (retval) so that we get:
> 
> Wpessimizing-move1.C:43:20: warning: moving a local object in a
> return statement prevents copy elision [-Wpessimizing-move]
> 43 |   return std::move (t);
>|  ~~^~~
> 
> certainly better than what input_location offers.  I'll go with that
> but
> I'll be touching the code today anyway and I'll look into using a
> fix-it
> hint.

Thanks.  Sorry if this is a bit of a digression from the main point of
the patch; I see this all as bonus material - maybe as a followup
assuming/once the real part of the patch is reviewed.


Am I right in thinking that the above code here ought to be simply:

  return t;

If so, then presumably we need the location of the start of parens, so
that we can suggest deletion from the start of the "std::move" through
to the open paren, and deletion of the close paren.

For a testcase for the fix-it hint, it's better to use something with a
length > 1 for the argument (to verify the range is correct), so it
would be something like:

Wpessimizing-move1.C:43:20: note: remove 'std::move' call:
43 |   return std::move (foo);
   |  ~~^
   |  ---   -

(which admittedly isn't the most readable presentation of that data,
but maybe that's fixable in diagnostic-show-locus.c).

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

[Tom de Vries]

On 08/20/2018 03:09 PM, Richard Biener wrote:
> On Fri, 17 Aug 2018, Tom de Vries wrote:
> 
>> I've rewritten the patch to work generically, not just for DW_AT_upper_bound,
>> and to reuse the code already there in add_scalar_info.
>>
>> OK for trunk?
>>
>> Thanks,
>> - Tom
>>
>> [debug] Fix handling of vlas in lto
>>
>> Atm, when running vla-1.c with -O0 -flto, we have:
>> ...
>> FAIL: gcc.dg/guality/vla-1.c -O0 -flto -fuse-linker-plugin \
>>   -fno-fat-lto-objects line 17 sizeof (a) == 6
>> ...
>>
>> The vla a[i + 1] in f1 is gimplified into:
>> ...
>> f1 (int i)
>> {
>>   char a[0:D.1922] [value-expr: *a.0];
>>   char[0:D.1922] * a.0;
>>
>>   D.1921 = i + 1;
>>   D.1926 = (sizetype) D.1921;
>>   a.0 = __builtin_alloca_with_align (D.1926, 8);
>> ...
>>
>> The early debug info for the upper bound of the type of vla a that we stream
>> out is:
>> ...
>>   DIE0: DW_TAG_subrange_type (0x7f85029a90f0)
>> DW_AT_upper_bound: location descriptor:
>>   (0x7f85029a9230) DW_OP_GNU_variable_value die -> 0 (0x7f85029a94b0), 0
>>   DIE0: DW_TAG_variable (0x7f85029a94b0)
>> DW_AT_name: "D.1922"
>> DW_AT_type: die -> 0 (0x7f85029a3d70)
>> DW_AT_artificial: 1
>> ...
>>
>> and in ltrans we have for that same upper bound:
>> ...
>>   DIE0: DW_TAG_subrange_type (0x7f5183b57d70)
>> DW_AT_upper_bound: die -> 0 (0x7f5183b576e0)
>>   DIE0: DW_TAG_variable (0x7f5183b576e0)
>> DW_AT_name: "D.4278"
>> DW_AT_abstract_origin: die -> label: vla_1.c.6719312a + 193 
>> (0x7f5183b57730)
>> ...
>> where D.4278 has abstract origin D.1922.
>>
>> The D.4278 die has no DW_AT_location, so when evaluting "sizeof (a)" in the
>> debugger, we can't find the information to get the value of D.4278, and the
>> debugger prints "".
>>
>> This patch fixes that by either:
>> - adding DW_AT_location to the referenced variable die, or
>> - instead of using a ref for the upper bound, using an exprloc.
>>
>> When changing gcc.dg/guality/guality.exp to run the usual flto flavours
>> "-fno-use-linker-plugin -flto-partition=none" and "-fuse-linker-plugin
>> -fno-fat-lto-objects" in combination with O0, Og, O1, O2, O3 and Os, this 
>> patch
>> fixes all (20) failures in vla-1.c, leaving only:
>> ...
>> No symbol "i" in current context.
>> UNSUPPORTED: gcc.dg/guality/vla-1.c  -O3 -flto -fno-use-linker-plugin \
>>   -flto-partition=none line 17 i == 5
>> 'a' has unknown type; cast it to its declared type
>> UNSUPPORTED: gcc.dg/guality/vla-1.c  -O3 -flto -fno-use-linker-plugin \
>>   -flto-partition=none line 17 sizeof (a) == 6
>> ...
>>
>> Bootstrapped and reg-tested on x86_64.
> 
> This looks OK to me.

Committed.

>  Note that with a gdb with DW_OP_variable_value 
> support we should be able to elide the VLA type in the concrete
> instance...
> 

Using this patch we elide the VLA type in the concrete instance for -O2
-flto:
...
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index dd8f438dfd3..5776185d9c6 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -23712,12 +23712,14 @@ gen_variable_die (tree decl, tree origin,
dw_die_ref context_die)
  && variably_modified_type_p
   (type, decl_function_context (decl_or_origin)))
{
+#if 0
  if (decl_by_reference_p (decl_or_origin))
add_type_attribute (var_die, TREE_TYPE (type),
TYPE_UNQUALIFIED, false,
context_die);
  else
add_type_attribute (var_die, type,
decl_quals (decl_or_origin),
false, context_die);
+#endif
}

  goto gen_variable_die_location;
...
and using master gdb (which supports DW_OP_variable_value, yay) we get:
...
7return a[0];  /* { dg-final { gdb-test . "sizeof (a)"
"6" } } */
vla-1.gdb:3: Error in sourced command file:
value has been optimized out
...

When evaluating sizeof (a) in gdb:
- we look for the concrete type die of a
- since that one is elided, we fall back on the type die of the abstract
  origin of a
- that one has an expr location for the upper bound containing a
  DW_OP_variable_value referring to a variable in early debug.
- that variable has no DW_AT_location, so we end up with "value has been
  optimized out"

AFAIU, that's roughly the issue discussed in
PR20426 - "gdb does not interpret DWARF annotating imported units fully"
 ( https://sourceware.org/bugzilla/show_bug.cgi?id=20426 ),.

Is my understanding correct that it's not yet clear how this should be
fixed?

Thanks,
- Tom

> Not sure how we should go forward there - use a configure test or
> simply tell people to update gdb?
> 
> Thanks,
> Richard.
> 
>> 2018-08-17  Tom de Vries  
>>
>>  * dwarf2out.c (add_scalar_info): Don't add reference to existing die
>>  unless the referenced die describes the added property using
>>  DW_AT_location or DW_AT_const_value.  Fall back to exprloc case.
>>  Ot

Re: C++ PATCH for c++/86981, implement -Wpessimizing-move

[Marek Polacek]

On Mon, Aug 20, 2018 at 07:42:10PM -0400, David Malcolm wrote:
> On Mon, 2018-08-20 at 18:54 -0400, Marek Polacek wrote:
> > On Mon, Aug 20, 2018 at 05:18:49PM -0400, David Malcolm wrote:
> > > As of r263675 it's now possible to tell the diagnostics subsystem
> > > that
> > > the warning and note are related by using an auto_diagnostic_group
> > > instance [1], so please can this read:
> > > 
> > >   if (can_do_nrvo_p (arg, functype))
> > > {
> > >   auto_diagnostic_group d;
> > >   if (warning (OPT_Wpessimizing_move, "moving a local
> > > object "
> > >"in a return statement prevents copy
> > > elision"))
> > > inform (input_location, "remove %
> > > call");
> > > }
> > 
> > Sure:
> > 
> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> Thanks!  The changed part looks good, but I can't really speak to the
> rest of the patch.
> 
> (BTW, could we emit a fix-it hint as part of that "inform" call?  How
> good is the location information at this point?)

Thanks, I can actually use location_of (retval) so that we get:

Wpessimizing-move1.C:43:20: warning: moving a local object in a return 
statement prevents copy elision [-Wpessimizing-move]
43 |   return std::move (t);
   |  ~~^~~

certainly better than what input_location offers.  I'll go with that but
I'll be touching the code today anyway and I'll look into using a fix-it
hint.

Marek

Re: Richard Sandiford appointed Global Reviewer

[Richard Sandiford]

David Edelsohn  writes:
>   I am pleased to announce that the GCC Steering Committee has
> appointed Richard Sandiford as a Global Reviewer.
>
>   Please join me in congratulating Richard on his new role.
> Richard, please update your listing in the MAINTAINERS file.

Thanks David, and thanks SC!  I've installed the patch below.

Richard


2018-08-21  Richard Sandiford  

* MAINTAINERS: Add self to global reviewers list.

Index: MAINTAINERS
===
--- MAINTAINERS 2018-08-21 14:47:08.771159612 +0100
+++ MAINTAINERS 2018-08-21 14:47:22.635042692 +0100
@@ -29,6 +29,7 @@ Michael Meissner  

 David S. Miller
 Joseph Myers   
+Richard Sandiford  
 Bernd Schmidt  
 Ian Lance Taylor   
 Jim Wilson 

Re: [PATCH][RFC] Add gimple-cfg.h.

[Richard Biener]

On Tue, Aug 21, 2018 at 2:58 PM Martin Liška  wrote:
>
> Hi.
>
> I'm considering adding couple of new gimple-related functions that
> are related to gswitch statement.
>
> Is it a good idea?

Just add them to tree-cfg.h?  That's what should be really called
gimple-cfg.h these days...

Richard.

> Martin
>
> gcc/ChangeLog:
>
> 2018-08-06  Martin Liska  
>
> * gimple-cfg.h: New file.
> * ipa-fnsummary.c (set_switch_stmt_execution_predicate):
> Use gimple_switch_edge (and gimple_switch_default_edge).
> * tree-switch-conversion.c (switch_conversion::collect): Likewise.
> (switch_decision_tree::compute_cases_per_edge): Likewise.
> (switch_decision_tree::analyze_switch_statement): Likewise.
> (switch_decision_tree::try_switch_expansion): Likewise.
> ---
>  gcc/gimple-cfg.h | 44 
>  gcc/ipa-fnsummary.c  |  7 +++---
>  gcc/tree-switch-conversion.c | 38 ---
>  3 files changed, 62 insertions(+), 27 deletions(-)
>  create mode 100644 gcc/gimple-cfg.h
>
>

Re: [PATCH] RFC: Refactor std::tuple constraints

[Ville Voutilainen]

On 21 August 2018 at 12:17, Jonathan Wakely  wrote:
> I think this is slightly simpler and easier to maintain, but I'd like
> to hear other views, especially from Ville who added most of these
> constraints and does most of the work to maintain std::tuple.

+1, OK.

Re: [PATCH] Merge Ignore and Deprecated in .opt files.

[Martin Liška]

On 08/18/2018 12:24 AM, Segher Boessenkool wrote:
> Hi!
> 
> On Thu, Aug 16, 2018 at 11:18:15AM +0200, Martin Liška wrote:
>> On 08/15/2018 06:38 PM, Joseph Myers wrote:
>>> On Wed, 15 Aug 2018, Martin Liška wrote:
>>>
 Ok, so you have very similar opinion as Jakub. Thus I'm sending new 
 version that preserves status quo, it only does:
>>>
>>> This is removing RejectNegative from some Deprecated options.  Won't that 
>>> result in the -fno-* variants of those options starting to be accepted, 
>>> when they never were accepted when the positive versions of the options 
>>> did something useful?
>>>
>>
>> That's not intended, I fixed that. It the patch acceptable in form in which 
>> it is?
>>
>> Thanks,
>> Martin
>>
> 
>> >From 0a7d5cd6cd6ca0586a350b95cd8f6ded095ba9c8 Mon Sep 17 00:00:00 2001
>> From: marxin 
>> Date: Wed, 18 Jul 2018 13:40:24 +0200
>> Subject: [PATCH] Merge Ignore and Deprecated in .opt files.
>>
>> gcc/ChangeLog:
>>
>> 2018-07-18  Martin Liska  
>>
>>  * common.opt: Remove Warn, Init and Report for options with
>> Ignore/Deprecated flag. Warning is done automatically for
>> Deprecated flags.
> 
> Too much indent.  Two spaces after a full stop.

Thanks.

> 
> Removing Init is *wrong* as far as I see; it changes things, anyway.
> Could you not have done this as a separate patch?

It's already in, but it should be fine. Note that I added check into
opt-functions.awk where I can't use Var with Deprecated or Ignore attribute.
Thus removal of Init should be fine. Or am I wrong?

> 
>>  * config/rs6000/rs6000.opt: Likewise.
> 
>> diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt
>> index 25a4883b161..b07f7f7e833 100644
>> --- a/gcc/config/rs6000/rs6000.opt
>> +++ b/gcc/config/rs6000/rs6000.opt
>> @@ -483,8 +483,9 @@ mcrypto
>>  Target Report Mask(CRYPTO) Var(rs6000_isa_flags)
>>  Use ISA 2.07 Category:Vector.AES and Category:Vector.SHA2 instructions.
>>  
>> +; We can't use Ignore flag because DIRECT_MOVE mask is still used.
>>  mdirect-move
>> -Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) Ignore Warn(%qs 
>> is deprecated)
>> +Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) Warn(%qs is 
>> deprecated)
> 
> This is not described in the changelog.
> 
> I don't understand what it means either; did you change the semantics of
> the "Ignore" flag?  It worked just fine before, and I don't know if it
> still does :-/
> 
> [ Please cc: the rs6000 maintainers when you change rs6000 code.  Thanks! ].

Sorry for that I forgot to add you to comment about. It's again related to 
sanity check
of Ignore attribute. This case is bit problematic, so I tend to revert it and 
remove
sanity check about 'Warn' attribute of an optioned when combined with 'Ignore'.

Martin

> 
> 
> Segher
> 

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

[Paul Hua]

Hi, Qing,

The cfarm machine gcc23 can build mips64el successful, configure with
"../configure --prefix=/opt/gcc-9 MISSING=texinfo MAKEINFO=missing
--target=mips64el-linux-gnu --enable-languages=c,c++
On Tue, Aug 21, 2018 at 7:02 AM Qing Zhao  wrote:
>
> Hi, Paul,
>
> I was trying to repeat this issue on a mips machine today, but failed…
>
> the only mips machines I can access are those in gcc compile farm, I chose 
> gcc22, but failed to build GCC on this machine.
>
> do you know any other machine in gcc compile farm that can repeat this issue?
>
> thanks a lot.
>
> Qing
> > On Aug 17, 2018, at 10:43 PM, Paul Hua  wrote:
> >
> > Hi Qing:
> >
> >>
> >> the change has been committed as:
> >> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=263563 
> >> 
> >>
> >> Qing
> >>
> >
> > The strcmpopt_6.c test still fails on mips64el target.
> >
> > gcc.dg/strcmpopt_6.c: memcmp found 4 times
> > FAIL: gcc.dg/strcmpopt_6.c scan-assembler-times memcmp 2
> >
> >
> > The mips asm output have ".reloc" info.
> >
> > -
> >ld  $5,%got_page(.LC0)($28)
> >ld  $25,%call16(memcmp)($28)
> >li  $6,3# 0x3
> >sd  $31,8($sp)
> >.reloc  1f,R_MIPS_JALR,memcmp
> > 1:  jalr$25
> >daddiu  $5,$5,%got_ofst(.LC0)
> > 
> >
> > scan-assembler find "4" times.
> >
> > Thanks
> > Paul Hua
>

[PATCH][RFC] Add gimple-cfg.h.

[Martin Liška]

Hi.

I'm considering adding couple of new gimple-related functions that
are related to gswitch statement.

Is it a good idea?
Martin

gcc/ChangeLog:

2018-08-06  Martin Liska  

* gimple-cfg.h: New file.
* ipa-fnsummary.c (set_switch_stmt_execution_predicate):
Use gimple_switch_edge (and gimple_switch_default_edge).
* tree-switch-conversion.c (switch_conversion::collect): Likewise.
(switch_decision_tree::compute_cases_per_edge): Likewise.
(switch_decision_tree::analyze_switch_statement): Likewise.
(switch_decision_tree::try_switch_expansion): Likewise.
---
 gcc/gimple-cfg.h | 44 
 gcc/ipa-fnsummary.c  |  7 +++---
 gcc/tree-switch-conversion.c | 38 ---
 3 files changed, 62 insertions(+), 27 deletions(-)
 create mode 100644 gcc/gimple-cfg.h


diff --git a/gcc/gimple-cfg.h b/gcc/gimple-cfg.h
new file mode 100644
index 000..1e95338bede
--- /dev/null
+++ b/gcc/gimple-cfg.h
@@ -0,0 +1,44 @@
+/* Gimple IR definitions related to CFG.
+
+   Copyright (C) 2007-2018 Free Software Foundation, Inc.
+   Contributed by Martin Liska 
+
+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
+.  */
+
+#ifndef GCC_GIMPLE_CFG_H
+#define GCC_GIMPLE_CFG_H
+
+/* Return the edge that belongs to label numbered INDEX
+   of a switch statement.  */
+
+static inline edge
+gimple_switch_edge (gswitch *gs, unsigned index)
+{
+  tree label = CASE_LABEL (gimple_switch_label (gs, index));
+  return find_edge (gimple_bb (gs), label_to_block (label));
+}
+
+/* Return the default edge of a switch statement.  */
+
+static inline edge
+gimple_switch_default_edge (gswitch *gs)
+{
+  tree label = CASE_LABEL (gimple_switch_label (gs, 0));
+  return find_edge (gimple_bb (gs), label_to_block (label));
+}
+
+#endif  /* GCC_GIMPLE_CFG_H */
diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
index a8fc2c2df9a..2991e647fc4 100644
--- a/gcc/ipa-fnsummary.c
+++ b/gcc/ipa-fnsummary.c
@@ -56,7 +56,10 @@ along with GCC; see the file COPYING3.  If not see
 #include "coretypes.h"
 #include "backend.h"
 #include "tree.h"
+#include "cfganal.h"
 #include "gimple.h"
+#include "tree-cfg.h"
+#include "gimple-cfg.h"
 #include "alloc-pool.h"
 #include "tree-pass.h"
 #include "ssa.h"
@@ -68,9 +71,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-inline.h"
 #include "gimple-pretty-print.h"
 #include "params.h"
-#include "cfganal.h"
 #include "gimple-iterator.h"
-#include "tree-cfg.h"
 #include "tree-ssa-loop-niter.h"
 #include "tree-ssa-loop.h"
 #include "symbol-summary.h"
@@ -1291,7 +1292,7 @@ set_switch_stmt_execution_predicate (struct ipa_func_body_info *fbi,
   tree min, max;
   predicate p;
 
-  e = find_edge (bb, label_to_block (CASE_LABEL (cl)));
+  e = gimple_switch_edge (last, case_idx);
   min = CASE_LOW (cl);
   max = CASE_HIGH (cl);
 
diff --git a/gcc/tree-switch-conversion.c b/gcc/tree-switch-conversion.c
index 9a594a01fc4..492cd365a30 100644
--- a/gcc/tree-switch-conversion.c
+++ b/gcc/tree-switch-conversion.c
@@ -29,6 +29,8 @@ Software Foundation, 51 Franklin Street, Fifth Floor, Boston, MA
 #include "insn-codes.h"
 #include "rtl.h"
 #include "tree.h"
+#include "cfganal.h"
+#include "tree-cfg.h"
 #include "gimple.h"
 #include "cfghooks.h"
 #include "tree-pass.h"
@@ -40,8 +42,8 @@ Software Foundation, 51 Franklin Street, Fifth Floor, Boston, MA
 #include "fold-const.h"
 #include "varasm.h"
 #include "stor-layout.h"
-#include "cfganal.h"
 #include "gimplify.h"
+#include "gimple-cfg.h"
 #include "gimple-iterator.h"
 #include "gimplify-me.h"
 #include "tree-cfg.h"
@@ -78,7 +80,6 @@ switch_conversion::collect (gswitch *swtch)
   unsigned int i;
   edge e, e_default, e_first;
   edge_iterator ei;
-  basic_block first;
 
   m_switch = swtch;
 
@@ -87,9 +88,8 @@ switch_conversion::collect (gswitch *swtch)
  Collect the bits we can deduce from the CFG.  */
   m_index_expr = gimple_switch_index (swtch);
   m_switch_bb = gimple_bb (swtch);
-  m_default_bb
-= label_to_block (CASE_LABEL (gimple_switch_default_label (swtch)));
-  e_default = find_edge (m_switch_bb, m_default_bb);
+  e_default = gimple_switch_default_edge (swtch);
+  m_default_bb = e_default->dest;
   m_default_prob = e_default->probability;
   m_default_count = e_default->count ();
   FOR_EACH_EDGE (e, ei, m_switch_bb->succs)
@@ -120,1

Re: [RFC][debug] Add -greadable-dwarf

[Tom de Vries]

Hi,

That solution is limited: it does not show the extra information in the
dump from an executable (which is the only way to see how things look
like post-linking).

Is your concern leaking compiler internals into the dwarf info?

Thanks,
- Tom

On 08/21/2018 06:53 AM, Jason Merrill wrote:
> How about adding this name to a -dA comment instead of the actual dwarf?
> 
> On Tue, Aug 21, 2018, 12:59 AM Richard Biener  > wrote:
> 
> On Wed, 15 Aug 2018, Tom de Vries wrote:
> 
> > Hi,
> >
> > This patch adds option -greadable-dwarf.  In absence of an
> DW_AT_comment
> > attribute,
> 
> What's a DW_AT_comment attribute?  I don't see this mentioned in the
> patch.
> 
> > it sets the DW_AT_name attribute of dies that otherwise do not get
> > that attribute, to make it easier to figure out what the die is
> describing.
> >
> > The option exports the names of artificial variables:
> > ...
> >  DIE    0: DW_TAG_variable (0x7fa934dd54b0)
> > +  DW_AT_name: "D.1922"
> >    DW_AT_type: die -> 0 (0x7fa934dd0d70)
> >    DW_AT_artificial: 1
> >
> > ...
> > which can be traced back to gimple dumps:
> > ...
> >   char a[0:D.1922] [value-expr: *a.0];
> > ...
> >
> > Furthermore, it adds names to external references:
> > ...
> >  DIE    0: DW_TAG_subprogram (0x7fa88b9650f0)
> > +DW_AT_name: "main"
> >  DW_AT_abstract_origin: die -> label: vla_1.c.6719312a + 29
> (0x7fa88b965140)
> > ...
> >
> > This is an undocumented developer-only option, because using this
> option may
> > change behaviour of dwarf consumers, f.i., gdb shows the
> artificial variables:
> > ...
> > (gdb) info locals
> > a = 0x7fffda90 "\005"
> > D.4278 = 
> > ...
> >
> > Any comments?
> 
> The idea is OK I guess but I'd call it -gforce-named-dies instead
> of -greadable-dwarf.  It also goes only half-way since it doesn't
> add names to DECL_NAMELESS vars.
> 
> There doesn't seem to be a convenient place to
> 
> > Thanks,
> > - Tom
> >
> > [debug] Add -greadable-dwarf
> >
> > 2018-08-15  Tom de Vries  mailto:tdevr...@suse.de>>
> >
> >       * common.opt (greadable-dwarf): Add option.
> >       * dwarf2out.c (add_name_and_src_coords_attributes): Add
> param. Add name
> >       for artifical decls.
> >       (add_decl_name): New function.
> >       (dwarf2out_register_external_die): Add name to external
> reference die.
> >
> > ---
> >  gcc/common.opt  |  5 +
> >  gcc/dwarf2out.c | 24 +---
> >  2 files changed, 26 insertions(+), 3 deletions(-)
> >
> > diff --git a/gcc/common.opt b/gcc/common.opt
> > index b2f2215ecc6..6e5e0558e49 100644
> > --- a/gcc/common.opt
> > +++ b/gcc/common.opt
> > @@ -2972,6 +2972,11 @@ gstrict-dwarf
> >  Common Driver Report Var(dwarf_strict) Init(0)
> >  Don't emit DWARF additions beyond selected version.
> > 
> > +greadable-dwarf
> > +Common Driver Undocumented Report Var(flag_readable_dwarf) Init(0)
> > +Make generated dwarf more readable, at the cost of space and
> exposing compiler
> > +internals.
> > +
> >  gtoggle
> >  Common Driver Report Var(flag_gtoggle)
> >  Toggle debug information generation.
> > diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> > index 4b63cbd8a1e..8c6b4372874 100644
> > --- a/gcc/dwarf2out.c
> > +++ b/gcc/dwarf2out.c
> > @@ -3824,7 +3824,9 @@ static void add_prototyped_attribute
> (dw_die_ref, tree);
> >  static dw_die_ref add_abstract_origin_attribute (dw_die_ref, tree);
> >  static void add_pure_or_virtual_attribute (dw_die_ref, tree);
> >  static void add_src_coords_attributes (dw_die_ref, tree);
> > -static void add_name_and_src_coords_attributes (dw_die_ref, tree,
> bool = false);
> > +static void add_name_and_src_coords_attributes (dw_die_ref, tree,
> bool = false,
> > +                                             bool = false);
> > +static void add_decl_name (dw_die_ref, tree);
> >  static void add_discr_value (dw_die_ref, dw_discr_value *);
> >  static void add_discr_list (dw_die_ref, dw_discr_list_ref);
> >  static inline dw_discr_list_ref AT_discr_list (dw_attr_node *);
> > @@ -6022,6 +6024,8 @@ dwarf2out_register_external_die (tree decl,
> const char *sym,
> >    else
> >      equate_decl_number_to_die (decl, die);
> > 
> > +  if (flag_readable_dwarf)
> > +    add_decl_name (die, decl);
> 
> Please use add_name_and_src_coords_attributes directly.
> 
> >    /* Add a reference to the DIE providing early debug at $sym +
> off.  */
> >    add_AT_external_die_ref (die, DW_AT_abstract_origin, sym, off);
> >  }
> > @@ -21269,7 +21273,8 @@ add_linkage_name (dw_die_ref die, tree decl)
> > 

Re: [RFC][debug] Add -greadable-dwarf

[Tom de Vries]

On 08/20/2018 02:59 PM, Richard Biener wrote:
> On Wed, 15 Aug 2018, Tom de Vries wrote:
> 
>> Hi,
>>
>> This patch adds option -greadable-dwarf.  In absence of an DW_AT_comment
>> attribute,
> 
> What's a DW_AT_comment attribute?  I don't see this mentioned in the
> patch.
> 

It's a hypothetical DWARF attribute, that I would have preferred to use
instead of DW_AT_name.

>> it sets the DW_AT_name attribute of dies that otherwise do not get
>> that attribute, to make it easier to figure out what the die is describing.
>>
>> The option exports the names of artificial variables:
>> ...
>>  DIE0: DW_TAG_variable (0x7fa934dd54b0)
>> +  DW_AT_name: "D.1922"
>>DW_AT_type: die -> 0 (0x7fa934dd0d70)
>>DW_AT_artificial: 1
>>
>> ...
>> which can be traced back to gimple dumps:
>> ...
>>   char a[0:D.1922] [value-expr: *a.0];
>> ...
>>
>> Furthermore, it adds names to external references:
>> ...
>>  DIE0: DW_TAG_subprogram (0x7fa88b9650f0)
>> +DW_AT_name: "main"
>>  DW_AT_abstract_origin: die -> label: vla_1.c.6719312a + 29 (0x7fa88b965140)
>> ...
>>
>> This is an undocumented developer-only option, because using this option may
>> change behaviour of dwarf consumers, f.i., gdb shows the artificial 
>> variables:
>> ...
>> (gdb) info locals
>> a = 0x7fffda90 "\005"
>> D.4278 = 
>> ...
>>
>> Any comments?
> 
> The idea is OK I guess but I'd call it -gforce-named-dies instead
> of -greadable-dwarf.

Done.

>  It also goes only half-way since it doesn't
> add names to DECL_NAMELESS vars.

I've tried to add that.

> There doesn't seem to be a convenient place to 
> 

?

>> Thanks,
>> - Tom
>>
>> [debug] Add -greadable-dwarf
>>
>> 2018-08-15  Tom de Vries  
>>
>>  * common.opt (greadable-dwarf): Add option.
>>  * dwarf2out.c (add_name_and_src_coords_attributes): Add param. Add name
>>  for artifical decls.
>>  (add_decl_name): New function.
>>  (dwarf2out_register_external_die): Add name to external reference die.
>>
>> ---
>>  gcc/common.opt  |  5 +
>>  gcc/dwarf2out.c | 24 +---
>>  2 files changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/gcc/common.opt b/gcc/common.opt
>> index b2f2215ecc6..6e5e0558e49 100644
>> --- a/gcc/common.opt
>> +++ b/gcc/common.opt
>> @@ -2972,6 +2972,11 @@ gstrict-dwarf
>>  Common Driver Report Var(dwarf_strict) Init(0)
>>  Don't emit DWARF additions beyond selected version.
>>  
>> +greadable-dwarf
>> +Common Driver Undocumented Report Var(flag_readable_dwarf) Init(0)
>> +Make generated dwarf more readable, at the cost of space and exposing 
>> compiler
>> +internals.
>> +
>>  gtoggle
>>  Common Driver Report Var(flag_gtoggle)
>>  Toggle debug information generation.
>> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
>> index 4b63cbd8a1e..8c6b4372874 100644
>> --- a/gcc/dwarf2out.c
>> +++ b/gcc/dwarf2out.c
>> @@ -3824,7 +3824,9 @@ static void add_prototyped_attribute (dw_die_ref, 
>> tree);
>>  static dw_die_ref add_abstract_origin_attribute (dw_die_ref, tree);
>>  static void add_pure_or_virtual_attribute (dw_die_ref, tree);
>>  static void add_src_coords_attributes (dw_die_ref, tree);
>> -static void add_name_and_src_coords_attributes (dw_die_ref, tree, bool = 
>> false);
>> +static void add_name_and_src_coords_attributes (dw_die_ref, tree, bool = 
>> false,
>> +bool = false);
>> +static void add_decl_name (dw_die_ref, tree);
>>  static void add_discr_value (dw_die_ref, dw_discr_value *);
>>  static void add_discr_list (dw_die_ref, dw_discr_list_ref);
>>  static inline dw_discr_list_ref AT_discr_list (dw_attr_node *);
>> @@ -6022,6 +6024,8 @@ dwarf2out_register_external_die (tree decl, const char 
>> *sym,
>>else
>>  equate_decl_number_to_die (decl, die);
>>  
>> +  if (flag_readable_dwarf)
>> +add_decl_name (die, decl);
> 
> Please use add_name_and_src_coords_attributes directly.
> 

Done.

>>/* Add a reference to the DIE providing early debug at $sym + off.  */
>>add_AT_external_die_ref (die, DW_AT_abstract_origin, sym, off);
>>  }
>> @@ -21269,7 +21273,8 @@ add_linkage_name (dw_die_ref die, tree decl)
>>  
>>  static void
>>  add_name_and_src_coords_attributes (dw_die_ref die, tree decl,
>> -bool no_linkage_name)
>> +bool no_linkage_name,
>> +bool no_src_coords_attributes)
>>  {
>>tree decl_name;
>>  
>> @@ -21279,12 +21284,19 @@ add_name_and_src_coords_attributes (dw_die_ref 
>> die, tree decl,
>>const char *name = dwarf2_name (decl, 0);
>>if (name)
>>  add_name_attribute (die, name);
>> -  if (! DECL_ARTIFICIAL (decl))
>> +  if (!no_src_coords_attributes && ! DECL_ARTIFICIAL (decl))
> 
> inconsistent spacing after !
> 

Done.

>>  add_src_coords_attributes (die, decl);
>>  
>>if (!no_linkage_name)
>>  add_linkage_name (die, decl);
>>  }
>> +  else if (flag_readable_dwarf && decl_name == NULL)
>> +{
>>

Re: C++ PATCH for c++/67012, c++/86942, detect invalid cases with function return type deduction

[Jason Merrill]

On Fri, Aug 17, 2018 at 2:17 PM, Marek Polacek  wrote:
> As I promised in ,
> this patch fixes a couple of invalid cases we weren't detecting.  It's got
> testcases from two PRs and another case I found out; they're intertwined so
> I think it makes sense to fix them in one go.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2018-08-16  Marek Polacek  
>
> PR c++/86942
> PR c++/67012
> * decl.c (grokdeclarator): Disallow functions with trailing return
> type with decltype(auto) as its type.  Also check the function if
> it's inner declarator doesn't exist.
>
> * g++.dg/cpp0x/auto52.C: New test.
> * g++.dg/cpp1y/auto-fn52.C: New test.
> * g++.dg/cpp1y/auto-fn53.C: New test.
> * g++.dg/cpp1y/auto-fn54.C: New test.
>
> diff --git gcc/cp/decl.c gcc/cp/decl.c
> index fa58bc4d2b3..8261f8e30e5 100644
> --- gcc/cp/decl.c
> +++ gcc/cp/decl.c
> @@ -11238,7 +11238,10 @@ grokdeclarator (const cp_declarator *declarator,
>
> /* Handle a late-specified return type.  */
> tree late_return_type = declarator->u.function.late_return_type;
> -   if (funcdecl_p)
> +   if (funcdecl_p
> +   /* This is the case e.g. for
> +  using T = auto () -> int.  */
> +   || inner_declarator == NULL)

Hmm, checking funcdecl_p here seems just wrong; these errors should be
the same regardless of whether this is declaring a function.  What
breaks if we just remove this condition?  The deduction guide errors
will need to be adjusted to handle the abstract declarator case, but
that looks like the only spot that would need fixing.

>   {
> if (tree auto_node = type_uses_auto (type))
>   {
> @@ -11270,6 +11273,18 @@ grokdeclarator (const cp_declarator *declarator,
>name, type);
> return error_mark_node;
>   }
> +   else if (is_auto (type)
> +&& (TYPE_IDENTIFIER (type)
> +== decltype_auto_identifier))

I think you want AUTO_IS_DECLTYPE here.

> + {
> +   if (funcdecl_p)
> + error ("%qs function with trailing return type has "
> +"% as its type rather than "
> +"plain %", name);
> +   else
> + error ("invalid use of %");
> +   return error_mark_node;
> + }
> tree tmpl = CLASS_PLACEHOLDER_TEMPLATE (auto_node);
> if (!tmpl)
>   if (tree late_auto = type_uses_auto (late_return_type))
> diff --git gcc/testsuite/g++.dg/cpp0x/auto52.C 
> gcc/testsuite/g++.dg/cpp0x/auto52.C
> index e69de29bb2d..9bfe7c754b5 100644
> --- gcc/testsuite/g++.dg/cpp0x/auto52.C
> +++ gcc/testsuite/g++.dg/cpp0x/auto52.C
> @@ -0,0 +1,6 @@
> +// PR c++/86942
> +// { dg-do compile { target c++11 } }
> +
> +using T = auto() -> int;
> +using U = void() -> int; // { dg-error "function with trailing return type 
> not declared with .auto." }
> +using W = auto(); // { dg-error "invalid use of .auto." }
> diff --git gcc/testsuite/g++.dg/cpp1y/auto-fn52.C 
> gcc/testsuite/g++.dg/cpp1y/auto-fn52.C
> index e69de29bb2d..e239bc27dc2 100644
> --- gcc/testsuite/g++.dg/cpp1y/auto-fn52.C
> +++ gcc/testsuite/g++.dg/cpp1y/auto-fn52.C
> @@ -0,0 +1,4 @@
> +// PR c++/67012
> +// { dg-do compile { target c++14 } }
> +
> +decltype(auto) f() -> int; // { dg-error "function with trailing return type 
> has" }
> diff --git gcc/testsuite/g++.dg/cpp1y/auto-fn53.C 
> gcc/testsuite/g++.dg/cpp1y/auto-fn53.C
> index e69de29bb2d..720aeeb215d 100644
> --- gcc/testsuite/g++.dg/cpp1y/auto-fn53.C
> +++ gcc/testsuite/g++.dg/cpp1y/auto-fn53.C
> @@ -0,0 +1,4 @@
> +// PR c++/86942
> +// { dg-do compile { target c++14 } }
> +
> +using T = decltype(auto) () -> int; // { dg-error "invalid use of" }
> diff --git gcc/testsuite/g++.dg/cpp1y/auto-fn54.C 
> gcc/testsuite/g++.dg/cpp1y/auto-fn54.C
> index e69de29bb2d..f3391ddfd75 100644
> --- gcc/testsuite/g++.dg/cpp1y/auto-fn54.C
> +++ gcc/testsuite/g++.dg/cpp1y/auto-fn54.C
> @@ -0,0 +1,3 @@
> +// { dg-do compile { target c++14 } }
> +
> +using T = int () -> decltype(auto); // { dg-error "function with trailing 
> return type not declared with .auto." }

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

[Joseph Myers]

On Tue, 21 Aug 2018, Martin Sebor wrote:

> I still believe it would be simpler, more robust, and safe
> to pass in a flag to either count bytes (the default, for
> all callers except sprintf %ls), or elements of the string
> type (for sprintf %ls).

But the correct thing to count for sprintf %ls is elements of the type 
expected by sprintf %ls.  That may not be the type of the string constant 
(and as this is in variable arguments, there isn't any implicit conversion 
to const wchar_t * like you would get with a function prototype - I'm not 
sure if anything ensures this code is only reached, regardless of 
language, if the argument does actually (after any casts present in the 
source code) point to the correct wchar_t type).

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

Re: C++ PATCH for c++/65043, missing narrowing warning with bool

[Jason Merrill]

On Wed, Aug 15, 2018 at 11:30 AM, Marek Polacek  wrote:
> We weren't complaining about narrowing when converting to bool because
> standard_conversion wasn't setting check_narrowing for the converting to bool
> case; it only sets it at the end of the function.  Moreover, check_narrowing
> wasn't catching real_type -> boolean_type at all, which is wrong;
> [dcl.init.list] says that a narrowing conversion is an implicit conversion 
> from
> a floating-point type to an integer type, and bool is an integer type.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2018-08-14  Marek Polacek  
>
> PR c++/65043
> * call.c (standard_conversion): Set check_narrowing.
> * typeck2.c (check_narrowing): Use CP_INTEGRAL_TYPE_P rather
> than comparing with INTEGER_TYPE.
>
> * g++.dg/concepts/pr67595.C: Add dg-warning.
> * g++.dg/cpp0x/Wnarrowing11.C: New test.
> * g++.dg/cpp0x/Wnarrowing12.C: New test.
> * g++.dg/cpp0x/rv-cast5.C: Add static_cast.
>
> diff --git gcc/cp/call.c gcc/cp/call.c
> index 62654a9e407..ded279a0f43 100644
> --- gcc/cp/call.c
> +++ gcc/cp/call.c
> @@ -1387,6 +1387,8 @@ standard_conversion (tree to, tree from, tree expr, 
> bool c_cast_p,
> conv->rank = cr_pbool;
>   if (NULLPTR_TYPE_P (from) && (flags & LOOKUP_ONLYCONVERTING))
> conv->bad_p = true;
> + if (flags & LOOKUP_NO_NARROWING)
> +   conv->check_narrowing = true;
>   return conv;
> }

Hmm, it might be better to fix this section to properly fall through
to the common check_narrowing code below.  OK either way.

Jason

Re: C++ PATCH for c++/86499, non-local lambda with a capture-default

[Jason Merrill]

On Tue, Aug 14, 2018 at 6:50 AM, Marek Polacek  wrote:
> This PR complains about us accepting a lambda with a capture-default
> at file scope, which seems to be forbidden.  This patch disallows
> such a use.  clang gives an error, so I did the same, but maybe we
> only want a pedwarn.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2018-08-13  Marek Polacek  
>
> PR c++/86499
> * parser.c (cp_parser_lambda_introducer): Give error if a non-local
> lambda has a capture-default.
>
> * g++.dg/cpp0x/lambda/lambda-non-local.C: New test.
> * g++.dg/cpp0x/lambda/lambda-this10.C: Adjust dg-error.
>
> diff --git gcc/cp/parser.c gcc/cp/parser.c
> index 8cfcd150705..6a760136f99 100644
> --- gcc/cp/parser.c
> +++ gcc/cp/parser.c
> @@ -10280,6 +10280,9 @@ cp_parser_lambda_introducer (cp_parser* parser, tree 
> lambda_expr)
>  {
>cp_lexer_consume_token (parser->lexer);
>first = false;
> +
> +  if (!(at_function_scope_p () || at_class_scope_p ()))

I think we want parsing_nsdmi() instead of at_class_scope_p(); other
uses in a class are ill-formed.  OK with that change.

> +   error ("non-local lambda expression cannot have a capture-default");
>  }
>
>while (cp_lexer_next_token_is_not (parser->lexer, CPP_CLOSE_SQUARE))
> diff --git gcc/testsuite/g++.dg/cpp0x/lambda/lambda-non-local.C 
> gcc/testsuite/g++.dg/cpp0x/lambda/lambda-non-local.C
> index e69de29bb2d..005f8f3888b 100644
> --- gcc/testsuite/g++.dg/cpp0x/lambda/lambda-non-local.C
> +++ gcc/testsuite/g++.dg/cpp0x/lambda/lambda-non-local.C
> @@ -0,0 +1,10 @@
> +// PR c++/86499
> +// { dg-do compile { target c++11 } }
> +
> +auto l1 = [=]{}; // { dg-error "non-local lambda expression cannot have a 
> capture-default" }
> +auto l2 = [&]{}; // { dg-error "non-local lambda expression cannot have a 
> capture-default" }
> +
> +namespace {
> +  auto l3 = [=]{}; // { dg-error "non-local lambda expression cannot have a 
> capture-default" }
> +  auto l4 = [&]{}; // { dg-error "non-local lambda expression cannot have a 
> capture-default" }
> +}
> diff --git gcc/testsuite/g++.dg/cpp0x/lambda/lambda-this10.C 
> gcc/testsuite/g++.dg/cpp0x/lambda/lambda-this10.C
> index b4b8e7201aa..e8e94771ef0 100644
> --- gcc/testsuite/g++.dg/cpp0x/lambda/lambda-this10.C
> +++ gcc/testsuite/g++.dg/cpp0x/lambda/lambda-this10.C
> @@ -1,4 +1,4 @@
>  // PR c++/54383
>  // { dg-do compile { target c++11 } }
>
> -auto foo = [&](int a) { return a > this->b; }; // { dg-error "this" }
> +auto foo = [&](int a) { return a > this->b; }; // { dg-error 
> "non-local|this" }

Re: [C++ Patch, obvious?] Change check_static_variable_definition return type to void

[Jason Merrill]

This doesn't seem obvious to me, since it is removing information from
the interface of the function.  But it is OK.

On Tue, Aug 21, 2018 at 10:20 PM, Paolo Carlini
 wrote:
> Hi,
>
> over the last couple of weeks I started auditing the front-end about
> problematic uses of permerror (say, the user passes -fpermissive and after
> the warning we immediately return error_mark_node anyway: the assembly most
> likely will not make sense) and noticed that we don't use anywhere the
> return value of check_static_variable_definition (luckily, because we are
> returning 1 both for error and permerror), thus the below, which just
> changes the return type to void and consistently adjusts the body. I mean to
> commit it as obvious after testing finishes...
>
> Thanks, Paolo.
>
> 
>

[C++ Patch, obvious?] Change check_static_variable_definition return type to void

[Paolo Carlini]

Hi,

over the last couple of weeks I started auditing the front-end about 
problematic uses of permerror (say, the user passes -fpermissive and 
after the warning we immediately return error_mark_node anyway: the 
assembly most likely will not make sense) and noticed that we don't use 
anywhere the return value of check_static_variable_definition (luckily, 
because we are returning 1 both for error and permerror), thus the 
below, which just changes the return type to void and consistently 
adjusts the body. I mean to commit it as obvious after testing finishes...


Thanks, Paolo.



2018-08-21  Paolo Carlini  

* decl.c (check_static_variable_definition): Change to return void.
Index: decl.c
===
--- decl.c  (revision 263695)
+++ decl.c  (working copy)
@@ -70,7 +70,7 @@ static void push_local_name (tree);
 static tree grok_reference_init (tree, tree, tree, int);
 static tree grokvardecl (tree, tree, tree, const cp_decl_specifier_seq *,
 int, int, int, bool, int, tree);
-static int check_static_variable_definition (tree, tree);
+static void check_static_variable_definition (tree, tree);
 static void record_unknown_type (tree, const char *);
 static tree builtin_function_1 (tree, tree, bool);
 static int member_function_or_else (tree, tree, enum overload_flags);
@@ -9531,25 +9531,24 @@ build_ptrmem_type (tree class_type, tree member_ty
 
 /* DECL is a VAR_DECL defined in-class, whose TYPE is also given.
Check to see that the definition is valid.  Issue appropriate error
-   messages.  Return 1 if the definition is particularly bad, or 0
-   otherwise.  */
+   messages.  */
 
-static int
+static void
 check_static_variable_definition (tree decl, tree type)
 {
   /* Avoid redundant diagnostics on out-of-class definitions.  */
   if (!current_class_type || !TYPE_BEING_DEFINED (current_class_type))
-return 0;
+;
   /* Can't check yet if we don't know the type.  */
-  if (dependent_type_p (type))
-return 0;
+  else if (dependent_type_p (type))
+;
   /* If DECL is declared constexpr, we'll do the appropriate checks
  in check_initializer.  Similarly for inline static data members.  */
-  if (DECL_P (decl)
+  else if (DECL_P (decl)
   && (DECL_DECLARED_CONSTEXPR_P (decl)
  || undeduced_auto_decl (decl)
  || DECL_VAR_DECLARED_INLINE_P (decl)))
-return 0;
+;
   else if (cxx_dialect >= cxx11 && !INTEGRAL_OR_ENUMERATION_TYPE_P (type))
 {
   if (!COMPLETE_TYPE_P (type))
@@ -9564,9 +9563,7 @@ check_static_variable_definition (tree decl, tree
error_at (DECL_SOURCE_LOCATION (decl),
  "in-class initialization of static data member %q#D of "
  "non-literal type", decl);
-  return 1;
 }
-
   /* Motion 10 at San Diego: If a static const integral data member is
  initialized with an integral constant expression, the initializer
  may appear either in the declaration (within the class), or in
@@ -9573,14 +9570,11 @@ check_static_variable_definition (tree decl, tree
  the definition, but not both.  If it appears in the class, the
  member is a member constant.  The file-scope definition is always
  required.  */
-  if (!ARITHMETIC_TYPE_P (type) && TREE_CODE (type) != ENUMERAL_TYPE)
-{
-  error_at (DECL_SOURCE_LOCATION (decl),
-   "invalid in-class initialization of static data member "
-   "of non-integral type %qT",
-   type);
-  return 1;
-}
+  else if (!ARITHMETIC_TYPE_P (type) && TREE_CODE (type) != ENUMERAL_TYPE)
+error_at (DECL_SOURCE_LOCATION (decl),
+ "invalid in-class initialization of static data member "
+ "of non-integral type %qT",
+ type);
   else if (!CP_TYPE_CONST_P (type))
 error_at (DECL_SOURCE_LOCATION (decl),
  "ISO C++ forbids in-class initialization of non-const "
@@ -9590,8 +9584,6 @@ check_static_variable_definition (tree decl, tree
 pedwarn (DECL_SOURCE_LOCATION (decl), OPT_Wpedantic,
 "ISO C++ forbids initialization of member constant "
 "%qD of non-integral type %qT", decl, type);
-
-  return 0;
 }
 
 /* *expr_p is part of the TYPE_SIZE of a variably-sized array.  If any

Re: [PATCH] Fix P81033 for FDEs in partitioned code.

[Richard Biener]

On Tue, Aug 21, 2018 at 11:45 AM Iain Sandoe  wrote:
>
>
> > On 20 Aug 2018, at 08:27, Richard Biener  wrote:
> >
> > On Tue, Aug 14, 2018 at 10:18 PM Mike Stump  wrote:
> >>
> >> On Aug 14, 2018, at 4:20 AM, Iain Sandoe  wrote:
> >>> When function sub-sections are enabled, Darwin’s assembler needs the FDE 
> >>> local start
> >>> label for each sub-section to follow a linker-visible one so that the FDE 
> >>> will be correctly
> >>> associated with the code of the subsection.
> >>>
> >>> The current code in final.c emits a linker-visible symbol, as needed by 
> >>> several targets.
> >>> However the local label used to define the FDE start precedes the 
> >>> linker-visible one
> >>> which, for Darwin causes it (the FDE start) to be associated with the 
> >>> previous linker-
> >>> visible symbol (or the section start if there isn’t one).  This applies 
> >>> regardless of the
> >>> actual address of the label, for toolchain assemblers that have strict 
> >>> interpretation of
> >>> the Darwin sub-sections-via-symbols ABI.
> >>>
> >>> The patch adds a new local label (analogous to the "LFBn” emitted for the 
> >>> regular
> >>> function starts) just after the linker-visible label emitted after 
> >>> switching text section.
> >>> The FDE second entry is made to point to this instead of the LcoldStartn 
> >>> one.  This
> >>> should be a no-op for targets using .cfi_ and for targets without 
> >>> sub-sections-via-symbols.
> >>>
> >>> Bootstrapped on x86_64 and i686 linux and on a number of Darwin platforms 
> >>> (from
> >>> i686-darwin9 to x86_64-darwin17).
> >>>
> >>> OK for trunk?
> >>> open branches? (although it's a regression on 8, it’s a latent wrong-code 
> >>> on all branches)
> >>
> >> I'm fine with the darwin aspects of it, but I think it needs 
> >> review/approval by final.c/dwarf people.
> >
> > The approach looks fine (though we have extra labels even for targets
> > that do not need it).  But,
> >
> > +  fde->dw_fde_second_begin = xstrdup (label);
> >
> > to me it looks like dw_fde_node is GC allocated so the above needs 
> > ggc_strdup.
>
> Revised below, (bootstrapped Linux and Darwin).
>
> OK to apply now?

OK.

Richard.

> thanks
> Iain
>
> gcc/
>
> PR bootstrap/81033
> PR target/81733
> PR target/52795
> * gcc/dwarf2out.c (FUNC_SECOND_SECT_LABEL): New.
> (dwarf2out_switch_text_section): Generate a local label for the second
> function sub-section and apply it as the second FDE start label.
> * gcc/final.c (final_scan_insn_1): Emit second FDE label after the 
> second
> sub-section start.
>
> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> index 236f199..6a66de7 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -297,6 +297,10 @@ static unsigned int rnglist_idx;
>  #define FUNC_BEGIN_LABEL   "LFB"
>  #endif
>
> +#ifndef FUNC_SECOND_SECT_LABEL
> +#define FUNC_SECOND_SECT_LABEL "LFSB"
> +#endif
> +
>  #ifndef FUNC_END_LABEL
>  #define FUNC_END_LABEL "LFE"
>  #endif
> @@ -1212,21 +1216,24 @@ static void set_cur_line_info_table (section *);
>  void
>  dwarf2out_switch_text_section (void)
>  {
> +  char label[MAX_ARTIFICIAL_LABEL_BYTES];
>section *sect;
>dw_fde_ref fde = cfun->fde;
>
>gcc_assert (cfun && fde && fde->dw_fde_second_begin == NULL);
>
> +  ASM_GENERATE_INTERNAL_LABEL (label, FUNC_SECOND_SECT_LABEL,
> +  current_function_funcdef_no);
> +
> +  fde->dw_fde_second_begin = ggc_strdup (label);
>if (!in_cold_section_p)
>  {
>fde->dw_fde_end = crtl->subsections.cold_section_end_label;
> -  fde->dw_fde_second_begin = crtl->subsections.hot_section_label;
>fde->dw_fde_second_end = crtl->subsections.hot_section_end_label;
>  }
>else
>  {
>fde->dw_fde_end = crtl->subsections.hot_section_end_label;
> -  fde->dw_fde_second_begin = crtl->subsections.cold_section_label;
>fde->dw_fde_second_end = crtl->subsections.cold_section_end_label;
>  }
>have_multiple_function_sections = true;
> diff --git a/gcc/final.c b/gcc/final.c
> index 842e5e0..6943c07 100644
> --- a/gcc/final.c
> +++ b/gcc/final.c
> @@ -2232,6 +2232,9 @@ final_scan_insn_1 (rtx_insn *insn, FILE *file, int 
> optimize_p ATTRIBUTE_UNUSED,
>   ASM_OUTPUT_LABEL (asm_out_file,
> IDENTIFIER_POINTER (cold_function_name));
>  #endif
> + if (dwarf2out_do_frame ()
> + && cfun->fde->dw_fde_second_begin != NULL)
> +   ASM_OUTPUT_LABEL (asm_out_file, 
> cfun->fde->dw_fde_second_begin);
> }
>   break;
>
>

Re: [PATCH] Fix P81033 for FDEs in partitioned code.

[Iain Sandoe]

> On 20 Aug 2018, at 08:27, Richard Biener  wrote:
> 
> On Tue, Aug 14, 2018 at 10:18 PM Mike Stump  wrote:
>> 
>> On Aug 14, 2018, at 4:20 AM, Iain Sandoe  wrote:
>>> When function sub-sections are enabled, Darwin’s assembler needs the FDE 
>>> local start
>>> label for each sub-section to follow a linker-visible one so that the FDE 
>>> will be correctly
>>> associated with the code of the subsection.
>>> 
>>> The current code in final.c emits a linker-visible symbol, as needed by 
>>> several targets.
>>> However the local label used to define the FDE start precedes the 
>>> linker-visible one
>>> which, for Darwin causes it (the FDE start) to be associated with the 
>>> previous linker-
>>> visible symbol (or the section start if there isn’t one).  This applies 
>>> regardless of the
>>> actual address of the label, for toolchain assemblers that have strict 
>>> interpretation of
>>> the Darwin sub-sections-via-symbols ABI.
>>> 
>>> The patch adds a new local label (analogous to the "LFBn” emitted for the 
>>> regular
>>> function starts) just after the linker-visible label emitted after 
>>> switching text section.
>>> The FDE second entry is made to point to this instead of the LcoldStartn 
>>> one.  This
>>> should be a no-op for targets using .cfi_ and for targets without 
>>> sub-sections-via-symbols.
>>> 
>>> Bootstrapped on x86_64 and i686 linux and on a number of Darwin platforms 
>>> (from
>>> i686-darwin9 to x86_64-darwin17).
>>> 
>>> OK for trunk?
>>> open branches? (although it's a regression on 8, it’s a latent wrong-code 
>>> on all branches)
>> 
>> I'm fine with the darwin aspects of it, but I think it needs review/approval 
>> by final.c/dwarf people.
> 
> The approach looks fine (though we have extra labels even for targets
> that do not need it).  But,
> 
> +  fde->dw_fde_second_begin = xstrdup (label);
> 
> to me it looks like dw_fde_node is GC allocated so the above needs ggc_strdup.

Revised below, (bootstrapped Linux and Darwin).

OK to apply now?

thanks
Iain

gcc/

PR bootstrap/81033
PR target/81733
PR target/52795
* gcc/dwarf2out.c (FUNC_SECOND_SECT_LABEL): New.
(dwarf2out_switch_text_section): Generate a local label for the second
function sub-section and apply it as the second FDE start label.
* gcc/final.c (final_scan_insn_1): Emit second FDE label after the 
second
sub-section start.

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 236f199..6a66de7 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -297,6 +297,10 @@ static unsigned int rnglist_idx;
 #define FUNC_BEGIN_LABEL   "LFB"
 #endif
 
+#ifndef FUNC_SECOND_SECT_LABEL
+#define FUNC_SECOND_SECT_LABEL "LFSB"
+#endif
+
 #ifndef FUNC_END_LABEL
 #define FUNC_END_LABEL "LFE"
 #endif
@@ -1212,21 +1216,24 @@ static void set_cur_line_info_table (section *);
 void
 dwarf2out_switch_text_section (void)
 {
+  char label[MAX_ARTIFICIAL_LABEL_BYTES];
   section *sect;
   dw_fde_ref fde = cfun->fde;
 
   gcc_assert (cfun && fde && fde->dw_fde_second_begin == NULL);
 
+  ASM_GENERATE_INTERNAL_LABEL (label, FUNC_SECOND_SECT_LABEL,
+  current_function_funcdef_no);
+
+  fde->dw_fde_second_begin = ggc_strdup (label);
   if (!in_cold_section_p)
 {
   fde->dw_fde_end = crtl->subsections.cold_section_end_label;
-  fde->dw_fde_second_begin = crtl->subsections.hot_section_label;
   fde->dw_fde_second_end = crtl->subsections.hot_section_end_label;
 }
   else
 {
   fde->dw_fde_end = crtl->subsections.hot_section_end_label;
-  fde->dw_fde_second_begin = crtl->subsections.cold_section_label;
   fde->dw_fde_second_end = crtl->subsections.cold_section_end_label;
 }
   have_multiple_function_sections = true;
diff --git a/gcc/final.c b/gcc/final.c
index 842e5e0..6943c07 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -2232,6 +2232,9 @@ final_scan_insn_1 (rtx_insn *insn, FILE *file, int 
optimize_p ATTRIBUTE_UNUSED,
  ASM_OUTPUT_LABEL (asm_out_file,
IDENTIFIER_POINTER (cold_function_name));
 #endif
+ if (dwarf2out_do_frame ()
+ && cfun->fde->dw_fde_second_begin != NULL)
+   ASM_OUTPUT_LABEL (asm_out_file, cfun->fde->dw_fde_second_begin);
}
  break;
 

Re: VRP: rewrite the division code (to handle corner cases including 0)

[Richard Biener]

On Wed, Aug 15, 2018 at 3:33 AM Aldy Hernandez  wrote:
>
> Howdy!
>
> In auditing the *_DIV_EXPR code I noticed that we were really botching
> some divisions where the divisor included 0.
>
> Particularly interesting was that we were botching something as simple
> as dividing by [0,0].  We were also incorrectly calculating things like
> [-2,-2] / [0, ], where we should have removed the 0 from the divisor.
>
> Also, the symbolic special casing could be handled by just treating
> symbolic ranges as [-MIN, +MAX] and letting the common code handle then.
>   Similarly for anti ranges, which actually never happen except for the
> constant case, since they've been normalized earlier.

Note we also have "mixed" symbolic (anti-)ranges like [0, a].  I don't think
we handled those before but extract_range_to_wide_ints may be improved
to handle them.  Likewise ranges_from_anti_range, ~[0, a] -> [-INF,
-1] u [a+1, +INF]
though not sure if that helps any case in practice.

> All in all, it was much easier to normalize all the symbolic ranges and
> treat everything generically by performing the division in two chunks...
> the negative numbers and the (non-zero) positive numbers.  And finally,
> unioning the results.  This makes everything much simpler to read with
> minimal special casing.

Yeah, nice work.  Few comments:

+  TYPE_OVERFLOW_UNDEFINED (expr_type),
+  TYPE_OVERFLOW_WRAPS (expr_type),

we no longer have the third state !UNDEFINED && !WRAPS so I suggest
to eliminate one for the other (just pass TYPE_OVERFLOW_UNDEFINED).

+  /* If we're definitely dividing by zero, there's nothing to do.  */
+  if (wide_int_range_zero_p (divisor_min, divisor_max, prec))
+return false;

I know we didn't do this before but for x / 0 we should compute UNDEFINED
as range.  With the current interfacing this special case would require handling
in the non-wide-int caller.

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

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

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

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

>
> (Yes, we can get something like ~[0,2] for a pointer for things like the
> following in libgcc:
>
>if (segment_arg == (void *) (uintptr_type) 1)
>  ...
>else if (segment_arg == (void *) (uintptr_type) 2)
>  return NULL;
>else if (segment_arg != NULL)
>  segment = (struct stack_segment *) segment_arg;
> )
>
> BTW, I am still not happy with the entire interface to wide-int-range.*,
> and have another pending patchset that will simplify things even
> further.  I think everyone will be pleased ;-).
>
> OK pending another round of tests?

The division related changes are OK, please split out & improve the nonnull
parts (and the POINTER_PLUS_EXPR check is already in as variant).

Richard.

>
> Aldy

[PATCH] Remove redundant { dg-do run } directives in tests

[Jonathan Wakely]

These tests accidentally had two dg-do directives. Only the second one
is needed.

* testsuite/26_numerics/bit/bitops.count/countl_one.cc: Remove
redundant dg-do directive.
* testsuite/26_numerics/bit/bitops.count/countl_zero.cc: Likewise.
* testsuite/26_numerics/bit/bitops.count/countr_one.cc: Likewise.
* testsuite/26_numerics/bit/bitops.count/countr_zero.cc: Likewise.
* testsuite/26_numerics/bit/bitops.count/popcount.cc: Likewise.

Tested x86_64-linux, committed to trunk.

commit 5e411aacf42df77e9d753b82ee2f54c65d396726
Author: Jonathan Wakely 
Date:   Tue Aug 21 10:20:03 2018 +0100

Remove redundant { dg-do run } directives in tests

These tests accidentally had two dg-do directives. Only the second one
is needed.

* testsuite/26_numerics/bit/bitops.count/countl_one.cc: Remove
redundant dg-do directive.
* testsuite/26_numerics/bit/bitops.count/countl_zero.cc: Likewise.
* testsuite/26_numerics/bit/bitops.count/countr_one.cc: Likewise.
* testsuite/26_numerics/bit/bitops.count/countr_zero.cc: Likewise.
* testsuite/26_numerics/bit/bitops.count/popcount.cc: Likewise.

diff --git a/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/countl_one.cc 
b/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/countl_one.cc
index c9ef5538538..bf8ab5b6046 100644
--- a/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/countl_one.cc
+++ b/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/countl_one.cc
@@ -15,8 +15,6 @@
 // with this library; see the file COPYING3.  If not see
 // .
 
-// { dg-do run { target c++11 } }
-
 // { dg-options "-std=gnu++2a" }
 // { dg-do compile { target c++2a } }
 
diff --git a/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/countl_zero.cc 
b/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/countl_zero.cc
index 7d619a7d697..8045ff5934c 100644
--- a/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/countl_zero.cc
+++ b/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/countl_zero.cc
@@ -15,8 +15,6 @@
 // with this library; see the file COPYING3.  If not see
 // .
 
-// { dg-do run { target c++11 } }
-
 // { dg-options "-std=gnu++2a" }
 // { dg-do compile { target c++2a } }
 
diff --git a/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/countr_one.cc 
b/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/countr_one.cc
index bb1ddd6234d..6e5f717d056 100644
--- a/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/countr_one.cc
+++ b/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/countr_one.cc
@@ -15,8 +15,6 @@
 // with this library; see the file COPYING3.  If not see
 // .
 
-// { dg-do run { target c++11 } }
-
 // { dg-options "-std=gnu++2a" }
 // { dg-do compile { target c++2a } }
 
diff --git a/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/countr_zero.cc 
b/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/countr_zero.cc
index d3b63aa4984..cc66145849b 100644
--- a/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/countr_zero.cc
+++ b/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/countr_zero.cc
@@ -15,8 +15,6 @@
 // with this library; see the file COPYING3.  If not see
 // .
 
-// { dg-do run { target c++11 } }
-
 // { dg-options "-std=gnu++2a" }
 // { dg-do compile { target c++2a } }
 
diff --git a/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/popcount.cc 
b/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/popcount.cc
index 2982cb19bbe..27dcc4acb01 100644
--- a/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/popcount.cc
+++ b/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/popcount.cc
@@ -15,8 +15,6 @@
 // with this library; see the file COPYING3.  If not see
 // .
 
-// { dg-do run { target c++11 } }
-
 // { dg-options "-std=gnu++2a" }
 // { dg-do compile { target c++2a } }
 

Re: PR libstdc++/68222 Hide safe iterator operators

[Jonathan Wakely]

On 18/08/18 22:31 +0200, François Dumont wrote:
Here is the new proposal. It is indeed possible to keep _Safe_iterator 
and just add a _Category template parameter to it.


While this is still a large patch (obviously, because it's changing a
lot!) I think this version is much easier to understand, and doesn't
add a whole new class unnecessarily. Thanks for updating it.

I introduce a friend declaration to access container _Base nested 
typedef from the safe iterator.


I review the safe const_iterator constructor from safe iterator. I now 
check if we are in the a const_iterator context so that compilers 
don't even try to consider this constructor when we are in an iterator 
context.


Nice.

I adapted _Safe_local_iterator the same way to keep consistency with 
_Safe_iterator and because I find the new design cleaner. It also 
fixes the same problem I fixed on _Safe_iterator when checking it 
iterator has been initialized using _MutableIterator() rather than 
_Iterator().


I stop overloading __get_distance for safe iterators or safe local 
iterators, it was useless. I prefer to introduce similar functions as 
members. Same for __get_distance_from_begin or __get_distance_to_end, 
and I move code in safe_iterator.tcc.


Tested under Linux x86_64 debug mode.

Ok to commit ?


OK for trunk, thanks again.

Re: P0646R1 for Debug mode

[Jonathan Wakely]

On 20/08/18 22:38 +0200, François Dumont wrote:
Right after I hit the send button I realized that I had forgotten to 
remove the definition of __cpp_lib_list_remove_return_type on the 
Debug forward_list side.


I haven't plan to define this macro in this context, I prefer to leave 
it to normal implementation.


So here is the updated patch.


Looks good, thanks for taking care of this.

Re: [PATCH][C++] Fix PR86763, wrong-code with TYPE_TYPELESS_STORAGE

[Richard Biener]

On Tue, 21 Aug 2018, Szabolcs Nagy wrote:

> On 20/08/18 10:33, Richard Biener wrote:
> > On Mon, 20 Aug 2018, Szabolcs Nagy wrote:
> > > On 02/08/18 09:30, Richard Biener wrote:
> ...
> > > > +  clock_gettime(CLOCK_REALTIME, &t);
> > > 
> > > this fails on targets without clock_gettime (baremetal)
> > > or targets that need -lrt at link time.
> > > 
> > > can we use something else here?
> > > e.g. time(&t) works on baremetal and does not depend on -lrt.
> > 
> > You'd have to check that the original bug still reproduces.  Since
> > it depends on scheduling chances may be that it is fragile :/
> > 
> > I suppose we could restrict the testcase to { *-*-linux }?
> > 
> 
> i decided to restrict it to linux:
> 
> clock_gettime is not available on some baremetal targets
> and may require -lrt on some non-linux targets.
> 
> OK for trunk?

OK.  OK also for backports.

Richard.

> gcc/testsuite/ChangeLog:
> 2018-08-21  Szabolcs Nagy  
> 
>   * g++.dg/torture/pr86763.C: Restrict to *-*-linux*.

[PATCH] RFC: Refactor std::tuple constraints

[Jonathan Wakely]

This refactors some std:tuple constraints, and fixes one bug. This
patch was generated with -b to ignore whitespace changes, so the
indentation is a little off (but it makes the patch smaller and easier
to read).

I haven't touched the std::tuple partial specialization yet,
this is just a demonstration of what I'm proposing. I'd like to either
remove the partial specialization completely (as proposed the other
day) or refactor its constraints the same way.

One of the main changes is to use __enable_if_t instead of having to
say typename enable_if<...>::type everywhere.

In a few places I replaced the _Dummy template parameter (which makes
the constructor dependent) with a bool _NotEmpty parameter which is
actually used in the constraints. There's no need for a separate
_Dummy that way. That simplifies the _TCC alias:

-  template using _TCC =
-_TC::value,
-_Elements...>;
+  template
+   using _TCC = _TC<_Cond, _Elements...>;

_TC::_NotSameTuple is replaced with a __valid_args function returning
a bool that is used as the argument for the _TMC alias template:

-  template using _TMC =
-  _TC<(sizeof...(_Elements) == sizeof...(_UElements))
- && (_TC<(sizeof...(_UElements)==1), _Elements...>::
- template _NotSameTuple<_UElements...>()),
-  _Elements...>;
+  template
+   using _TMC = _TC<__valid_args<_UElements...>(), _Elements...>;

This avoids instantiating one specialization of _TC just to call
_NotSameTuple in order to find out the bool argument for another
specialization of _TC.

The __valid_args function checks the pack sizes so several explicit
checks for sizeof...(_Elements) >= 1 can be removed.

The _TNTC alias can be removed completely, replaced by _TCC<_Size1>,
where _Size1 is a bool parameter that replaces a _Dummy parameter.

The "allocator-extended default constructor" was missing any
constraints on constructibility, so I added that (that's the bug fix).

I've reported an LWG issue that the "allocator-extended default
constructor" is not conditionally explicit. The patch doesn't add
that.

I'd also like to give _TC, _TMC, _TCC etc. more descriptive names,
make them all private, and rename _Elements to _Types and _UElements
to _UTypes (consistent with the standard) but that can be done in a
separate patch.

I think this is slightly simpler and easier to maintain, but I'd like
to hear other views, especially from Ville who added most of these
constraints and does most of the work to maintain std::tuple.



diff --git a/libstdc++-v3/include/std/tuple b/libstdc++-v3/include/std/tuple
index 56b97c25eed..05fce2e8cd0 100644
--- a/libstdc++-v3/include/std/tuple
+++ b/libstdc++-v3/include/std/tuple
@@ -469,13 +469,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 __not_>
  >::value;
}
-
-template
-static constexpr bool _NotSameTuple()
-{
-  return  __not_,
-__remove_cvref_t<_UElements>...>>::value;
-}
  };

  template
@@ -510,12 +503,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
{
  return true;
}
-
-template
-static constexpr bool _NotSameTuple()
-{
-  return true;
-}
  };

  /// Primary class template, tuple
@@ -533,6 +520,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
{
  return __and_...>::value;
}
+
static constexpr bool _ImplicitlyDefaultConstructibleTuple()
{
  return __and_<__is_implicitly_default_constructible<_Elements>...>
@@ -553,87 +541,86 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__and_...>::value;
}

+  // Constraint for tuple(_UTypes&&...) where sizeof...(_UTypes) == 1.
+  template
+   static constexpr bool __valid_args()
+   {
+ return sizeof...(_Elements) == 1
+   && !is_same>::value;
+   }
+
+  // Constraint for tuple(_UTypes&&...) where sizeof...(_UTypes) > 1.
+  template
+   static constexpr bool __valid_args()
+   { return (sizeof...(_Tail) + 2) == sizeof...(_Elements); }
+
public:
  template::
+   __enable_if_t<_TC2<_Dummy>::
   _ImplicitlyDefaultConstructibleTuple(),
-  bool>::type = true>
+bool> = true>
constexpr tuple()
: _Inherited() { }

  template::
+   __enable_if_t<_TC2<_Dummy>::
   _DefaultConstructibleTuple()
-  &&
-  !_TC2<_Dummy>::
+&& !_TC2<_Dummy>::
_ImplicitlyDefaultConstructibleTuple(),
-  bool>::type = false>
+bool> = false>
explicit constexpr tuple()
: _Inherited() { }

  // Shortcut for the cases where constructors taking _Elements...
  // need to be constrained.
-  tem

Re: [PATCH][C++] Fix PR86763, wrong-code with TYPE_TYPELESS_STORAGE

[Szabolcs Nagy]

On 20/08/18 10:33, Richard Biener wrote:

On Mon, 20 Aug 2018, Szabolcs Nagy wrote:

On 02/08/18 09:30, Richard Biener wrote:

...

+  clock_gettime(CLOCK_REALTIME, &t);


this fails on targets without clock_gettime (baremetal)
or targets that need -lrt at link time.

can we use something else here?
e.g. time(&t) works on baremetal and does not depend on -lrt.


You'd have to check that the original bug still reproduces.  Since
it depends on scheduling chances may be that it is fragile :/

I suppose we could restrict the testcase to { *-*-linux }?



i decided to restrict it to linux:

clock_gettime is not available on some baremetal targets
and may require -lrt on some non-linux targets.

OK for trunk?

gcc/testsuite/ChangeLog:
2018-08-21  Szabolcs Nagy  

* g++.dg/torture/pr86763.C: Restrict to *-*-linux*.

diff --git a/gcc/testsuite/g++.dg/torture/pr86763.C b/gcc/testsuite/g++.dg/torture/pr86763.C
index 79523b24850..8455ac9ce12 100644
--- a/gcc/testsuite/g++.dg/torture/pr86763.C
+++ b/gcc/testsuite/g++.dg/torture/pr86763.C
@@ -1,6 +1,6 @@
-// { dg-do run }
+// { dg-do run { target { *-*-linux* } } }
 // { dg-additional-options "-fschedule-insns2 -fstrict-aliasing" }
-// { dg-additional-options "-lrt" { target *-*-linux-gnu } }
+// { dg-additional-options "-lrt" }
 
 #include 
 #include 

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

[Richard Biener]

On Tue, 21 Aug 2018, Bernd Edlinger wrote:

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

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

Richard.

> 
> Bernd.
> 
> >>> I would just like the ability to get the length/size somehow
> >>> so that the questionable code that started us down this path
> >>> can be detected.  This is not just the sprintf(d, "%s", L"1")
> >>> kind of a mismatch but also the missing nul detection in cases
> >>> like:
> >> [ ... ]
> >> I know.  And ideally we'll be able to handle everything you want to.
> >> But we also have to realize that sometimes we may have to punt.
> >>
> >> Also remember that incremental progress is (usually) good.
> >>
> >>
> >>>
> >>>   const wchar_t a[1] = L"\x";
> >>>   strcpy(d, (char*)a);
> >> This touches on both the representational issue (excess elements in the
> >> initializer) and how to handle a non-terminated string.  Both are issues
> >> we're debating right now.
> >>
> >>>
> >>> I don't think this is necessarily an important use case to
> >>> optimize for but it is one that GCC optimizes already nonetheless,
> >>> and always has.  For example, this has always been folded into 4
> >>> by GCC and still is even with the patch:
> >>>
> >>>   const __WCHAR_TYPE__ wc[] = L"\x12345678";
> >>>
> >>>   int f (void)
> >>>   {
> >>>     const char *p = (char*)wc;
> >>>     return strlen (p);    // folded to 4
> >>>   }
> >>>
> >>> It's optimized because fold_const_call() relies on c_getstr()
> >>> which returns the underlying byte representation of the wide
> >>> string, and despite c_strlen() now trying to prevent it.
> >> And I think you're hitting on some of issues already raised in the thread.
> >>
> >> In this specific case though ISTM 4 is the right answer.  We casted to a
> >> char *, so that's what we should be counting.  Or am I missing
> >> something?  Also note that's the value we'd get from the strlen C
> >> library call IIUC.
> > 
> > It is the right answer.  My point is that this is optimized
> > but the change to c_strlen() prevents the same optimization
> > in other similar cases.  For example, GCC 6 folds this into
> > memcpy:
> > 
> >    __builtin_strcpy (d, (char*)L"\x12345678");
> > 
> > GCC 7 and 8 do too but get the byte count wrong (my bad).
> > 
> > Current trunk doesn't optimize it.  If restoring the original
> > behavior is the intent (and not just fixing the counting bug)
> > then c_strlen() should be fixed to fold this again.
> > 
> > (I'm not a fan of the strcpy to memcpy transformation because
> > it makes other things difficult but that's beside the point.)
> > 
> >>> The missing nul variant of the same test case isn't folded (it
> >>> ends up calling the library strlen) but the bug cannot be
> >>> detected using the modified c_strlen():
> >>>
> >>>   const __WCHAR_TYPE__ wc[1] = L"\x12345678";   // no nul
> >>>
> >>>   int f (void)
> >>>   {
> >>>     const char *p = (char*)wc;
> >>>     return strlen (p);    // not folded
> >>>   }
> >>>
> >>> To detect it, I'd have to use some other function, like
> >>> c_getstr().  I could certainly do that but I don't think
> >>> I should need to.
> >> And that's on the list of things we're going to try and address next.  I
> >> don't think it needs to be conflated with change which indicated to
> >> c_strlen how to count.
> > 
> > The two are one and the same: if c_strlen() doesn't count bytes
> > in arrays of wide characters (wchar_t, char16_t, or char32_t)
> > then the original GCC behavior is not restored and the missing
> > nul detection won't work for these "mixed type" cases.
> > 
> >> I do hope you're getting all these testcases recorded.  I'd even support
> >> installing them into the suite now, properly xfailed.  It's easier to
> >> see how any patch under consideration affects the wider set of tests.
> > 
> > I haven't been recording any of them --  I have no idea where
> > this is going.  As I've said, these patches prevent some of
> > the work I have already submitted.  That's why I have been
> > objecting to them, including adding the ELTSIZE argument to
> > c_strlen().
> > 
> > Martin
> 
> 

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

Re: [PATCH] Make strlen range computations more conservative

[Richard Biener]

On Mon, 20 Aug 2018, Bernd Edlinger wrote:

> 
> 
> On 08/20/18 12:23, Richard Biener wrote:
> > On Sun, 19 Aug 2018, Bernd Edlinger wrote:
> > 
> >> Hi,
> >>
> >>
> >> I rebased my range computation patch to current trunk,
> >> and updated it according to what was discussed here.
> >>
> >> That means get_range_strlen has already a parameter
> >> that is used to differentiate between ranges for warnings
> >> and ranges for code-gen.
> >>
> >> That is called "strict", in the 4-parameter overload
> >> and "fuzzy" in the internally used 7-parameter overload.
> >>
> >> So I added an "optimistic" parameter to my
> >> get_inner_char_array_unless_typecast helper function.
> >> That's it.
> >>
> >> Therefore at this time, there is only one warning regression
> >> in one test case and one xfailed warning test case fixed.
> >>
> >> So that is par on the warning regression side.
> >>
> >> The failed test case is gcc/testsuite/gcc.dg/pr83373.c which
> >> uses -fassume-zero-terminated-char-arrays, to enable the
> >> (unsafe) feedback from string-length information to VRP to
> >> suppress the warning.
> >>
> >> The 5 test cases that were designed to check the optimized
> >> tree dump have to use the new -fassume-zero-terminated-char-arrays
> >> option, but that is what we agreed upon.
> >>
> >> The patch is not dependent on any other patches.
> >>
> >>
> >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> >> Is it OK for trunk?
> > 
> > +  tree base = arg;
> > +  while (TREE_CODE (base) == ARRAY_REF
> > +|| TREE_CODE (base) == ARRAY_RANGE_REF
> > +|| TREE_CODE (base) == COMPONENT_REF)
> > +base = TREE_OPERAND (base, 0);
> > +
> > +  /* If this looks like a type cast don't assume anything.  */
> > +  if ((TREE_CODE (base) == MEM_REF
> > +   && (! integer_zerop (TREE_OPERAND (base, 1))
> > +  || TYPE_MAIN_VARIANT (TREE_TYPE (TREE_TYPE (TREE_OPERAND (base,
> > 0
> > + != TYPE_MAIN_VARIANT (TREE_TYPE (base
> > 
> > I'm not convinced you are testing anything useful here.
> > TREE_OPERAND (base, 1) might be a pointer which means it's type
> > doesn't have any semantics so you are testing the access type
> > against "random".  If you'd restrict this to ADDR_EXPRs and
> > look at the objects declared type then you'd still miss
> > type-changes from a dynamic type that is different from what
> > is declared.
> > 
> 
> This whole function is only used for warnings or if the test case
> asks for unsafe optimization via -ffassume-zero-terminated-char-arrays.
> 
> So yes, it is understood, but it has proven to be an oracle with
> 99.9% likelihood to give the right answer.
> 
> > So my conclusion is if you really want to not want to return
> > arg for things that look like a type cast then you have to
> > unconditionally return NULL_TREE.
> 
> Yes. :-)
> 
> > 
> > +  || TREE_CODE (base) == VIEW_CONVERT_EXPR
> > +  /* Or other stuff that would be handled by get_inner_reference.  */
> > 
> > simply use || handled_component_p (base) for the above and the rest
> > to be sure to handle everything that is not stripped above.
> > 
> > +  || TREE_CODE (base) == BIT_FIELD_REF
> > +  || TREE_CODE (base) == REALPART_EXPR
> > +  || TREE_CODE (base) == IMAGPART_EXPR)
> > +return NULL_TREE;
> > 
> 
> Yes, good point.
> 
> > Btw, you are always returning the passed arg or NULL_TREE so
> > formulating this as a predicate function makes uses easier.
> > Not sure why it is called "inner" char array?
> > 
> ;
> 
> Yes, in a previous version of this patch, this function actually
> walked towards the innermost array, and returned that, but I dropped
> that part, as it caused too many test cases regress.
> 
> So agreed, I think I will convert that to a _p function and think
> of a better name.
> 
> 
> > There do seem to be independently useful fixes in the patch that
> > I'd approve immediately.
> > 
> 
> Yes, I found some peanuts on my way.
> 
> For instance this fix for PR middle-end/86121 survives bootstrap on
> it's own, and fixes one xfail.
> 
> Is it OK for trunk?

Yes, that's OK for trunk.

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

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

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

Any example?

> But we can as well remove those test cases.
> 
> Bernd.

Re: [PATCH] Make GO string literals properly NUL terminated

[Richard Biener]

On Mon, 20 Aug 2018, Bernd Edlinger wrote:

> On 08/20/18 15:19, Richard Biener wrote:
> > On Mon, 20 Aug 2018, Bernd Edlinger wrote:
> > 
> >> On 08/20/18 13:01, Richard Biener wrote:
> >>> On Wed, Aug 1, 2018 at 3:05 PM Bernd Edlinger  
> >>> wrote:
> 
> 
> 
>  On 08/01/18 11:29, Richard Biener wrote:
> >
> > Hmm.  I think it would be nice if TREE_STRING_LENGTH would
> > match char[2] and TYPE_SIZE_UNIT even if that is inconvenient
> > for your check above.  Because the '\0' doesn't belong to the
> > string.  Then build_string internally appends a '\0' outside
> > of TREE_STRING_LENGTH.
> >
> 
>  Hmm. Yes, but the outside-0 byte is just one byte, not a wide
>  character.
> >>>
> >>> That could be fixed though (a wide 0 is just N 0s).  Add a elsz = 1
> >>> parameter to build_string and allocate as many extra 0s as needed.
> >>>
> >>> There are STRING_CSTs which are not string literals,
>  for instance attribute tags, Pragmas, asm constrants, etc.
>  They use the '\0' outside, and have probably no TREE_TYPE.
> 
> >
> >> So I would like to be able to assume that the STRING_CST objects
> >> are internally always generated properly by the front end.
> >
> > Yeah, I guess we need to define what "properly" is ;)
> >
>  Yes.
> 
> >> And that the ARRAY_TYPE of the string literal either has the
> >> same length than the TREE_STRING_LENGTH or if it is shorter,
> >> this is always exactly one (wide) character size less than 
> >> TREE_STRING_LENGTH
> >
> > I think it should be always the same...
> >
> 
>  One could not differentiate between "\0" without zero-termination
>  and "" with zero-termination, theoretically.
> >>>
> >>> Is that important?  Doesn't the C standard say how to parse string 
> >>> literals?
> >>>
>  We also have char x[100] = "ab";
>  that is TREE_STRING_LENGTH=3, and TYPE_SIZE_UNIT(TREE_TYPE(x)) = 100.
>  Of course one could create it with a TREE_STRING_LENGTH = 100,
>  but imagine char x[1000] = "ab"
> >>>
> >>> The question is more about TYPE_SIZE_UNIT (TREE_TYPE ("ab")) which I
> >>> hope matches "ab" and not 'x'.  If it matches 'x' then I'd rather have it
> >>> unconditionally be [], thus incomplete (because the literals "size" 
> >>> depends
> >>> on the context/LHS it is used on).
> >>>
> >>
> >> Sorry, but I must say, it is not at all like that.
> >>
> >> If I compile x.c:
> >> const char x[100] = "ab";
> >>
> >> and set a breakpoint at output_constant:
> >>
> >> Breakpoint 1, output_constant (exp=0x76ff9dc8, size=100, align=256,
> >>   reverse=false) at ../../gcc-trunk/gcc/varasm.c:4821
> >> 4821 if (size == 0 || flag_syntax_only)
> >> (gdb) p size
> >> $1 = 100
> >> (gdb) call debug(exp)
> >> "ab"
> >> (gdb) p *exp
> >> $2 = {base = {code = STRING_CST, side_effects_flag = 0, constant_flag = 1,
> >> (gdb) p exp->typed.type->type_common.size_unit
> >> $5 = (tree) 0x76ff9d80
> >> (gdb) call debug(exp->typed.type->type_common.size_unit)
> >> 100
> >> (gdb) p exp->string.length
> >> $6 = 3
> >> (gdb) p exp->string.str[0]
> >> $8 = 97 'a'
> >> (gdb) p exp->string.str[1]
> >> $9 = 98 'b'
> >> (gdb) p exp->string.str[2]
> >> $10 = 0 '\000'
> >> (gdb) p exp->string.str[3]
> >> $11 = 0 '\000'
> >>
> >>
> >> This is an important property of string_cst objects, that is used in 
> >> c_strlen:
> >>
> >> It folds c_strlen(&x[4]) directly to 0, because every byte beyond 
> >> TREE_STRING_LENGTH
> >> is guaranteed to be zero up to the type size.
> >>
> >> I would not have spent one thought on implementing an optimization like 
> >> that,
> >> but that's how it is right now.
> > 
> > Huh.  So somebody interpreted STRING_CSTs similar to CONSTRUCTORs aka
> > they have zero-padding up to its type size.  I don't see this documented
> > anywhere and it would suggest to "optimize" "ab\0\0\0\0" to "ab\0"
> > with appropriate TYPE_SIZE.
> > 
> > This is also a relatively new thing on trunk (coming out of the added
> > mem_size parameter of string_constant).  That this looks at the STRING_CST
> > type like
> > 
> >if (TREE_CODE (array) == STRING_CST)
> >  {
> >*ptr_offset = fold_convert (sizetype, offset);
> >if (mem_size)
> >  *mem_size = TYPE_SIZE_UNIT (TREE_TYPE (array));
> >return array;
> > 
> > I'd call simply a bug.  As said, interpretation of STRING_CSTs should
> > depend on the context.  And for
> > 
> 
> This use of the TYPE_SIZE_UNIT was pre-existing to r263607
> before that (but not in the gcc-8-branch) we had this in c_strlen:
> 
>HOST_WIDE_INT maxelts = strelts;
>tree type = TREE_TYPE (src);
>if (tree size = TYPE_SIZE_UNIT (type))
>  if (tree_fits_shwi_p (size))
>{
> maxelts = tree_to_uhwi (size);
> maxelts = maxelts / eltsize - 1;
>}
> 
> which I moved to string_constant and return the result through memsize.
> 
> It seems to

Re: [PATCH] Use c_getstr to get the format string in gimple-ssa-sprintf.c

[Richard Biener]

On Mon, 20 Aug 2018, Bernd Edlinger wrote:

> Hi,
> 
> 
> this simplifies get_format_string in gimple-ssa-sprintf.c
> to use c_getstr.
> 
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?

OK.

Thanks,
Richard.

Re: [PATCH, rs6000] Improve TREE_TYPE comparisons in fold_mergehl_helper()

[Richard Biener]

On Tue, Aug 21, 2018 at 12:07 AM Segher Boessenkool
 wrote:
>
> Hi Will,
>
> On Mon, Aug 20, 2018 at 04:40:35PM -0500, Will Schmidt wrote:
> > This is a follow-up to an earlier patch that enabled gimple folding of
> > vec_mergeh and vec_mergel for the float and double data types.
> >
> > Per feedback from Richard, use the types_compatible_p helper to ensure
> > we also catch any qualified types matching the V2DF_ or V4SF_ types.
>
> That looks fine; if no one hollers, please commit.  Thanks!
>
> I note we use lang_hooks.types_compatible_p a lot, which is a totally
> different thing?  How confusing :-/

Yeah, that's supposed to be used in target hooks used by frontends.

Richard.

>
> Segher
>
>
> > 2018-08-20  Will Schmidt  
> >
> >   * config/rs6000/rs6000.c (fold_mergehl_helper): Add types_compatible_p
> >   wrappers around TREE_TYPE comparisons.
> >
> > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> > index 97b922f..5f77afd 100644
> > --- a/gcc/config/rs6000/rs6000.c
> > +++ b/gcc/config/rs6000/rs6000.c
> > @@ -15135,13 +15135,15 @@ fold_mergehl_helper (gimple_stmt_iterator *gsi, 
> > gimple *stmt, int use_high)
> >tree permute_type;
> >if (INTEGRAL_TYPE_P (TREE_TYPE (lhs_type)))
> >  permute_type = lhs_type;
> >else
> >  {
> > -  if (TREE_TYPE (lhs_type) == TREE_TYPE (V2DF_type_node))
> > +  if (types_compatible_p (TREE_TYPE (lhs_type),
> > +   TREE_TYPE (V2DF_type_node)))
> >   permute_type = V2DI_type_node;
> > -  else if (TREE_TYPE (lhs_type) == TREE_TYPE (V4SF_type_node))
> > +  else if (types_compatible_p (TREE_TYPE (lhs_type),
> > +TREE_TYPE (V4SF_type_node)))
> >   permute_type = V4SI_type_node;
> >else
> >   gcc_unreachable ();
> >  }
> >tree_vector_builder elts (permute_type, VECTOR_CST_NELTS (arg0), 1);
> >