Re: [PATCH 2/3] Fix probability for bit-tests.

[Jeff Law]

On 08/03/2018 08:10 AM, marxin wrote:
> The patch set even probability to bit test based on number of
> cases which are handled on each edge.
> 
> gcc/ChangeLog:
> 
> 2018-08-06  Martin Liska  
> 
>   * tree-switch-conversion.c (bit_test_cluster::find_bit_tests):
> Add new argument to bit_test_cluster constructor.
>   (bit_test_cluster::emit): Set bits really number of values
> handlel by a test.
>   (bit_test_cluster::hoist_edge_and_branch_if_true): Add
> probability argument.
>   * tree-switch-conversion.h (struct bit_test_cluster):
> Add m_handles_entire_switch.
> 
> gcc/testsuite/ChangeLog:
> 
> 2018-08-06  Martin Liska  
> 
>   * gcc.dg/tree-ssa/switch-2.c: New test.
OK
jeff

Re: [PATCH 1/3] Fix probabilities for jump table (PR tree-optimization/86702).

[Jeff Law]

On 08/03/2018 06:58 AM, marxin wrote:
> The patch set even probability to jump tables based on number of
> cases which are handled on each edge.
> 
> gcc/ChangeLog:
> 
> 2018-08-06  Martin Liska  
> 
> PR tree-optimization/86702
>   * tree-switch-conversion.c (jump_table_cluster::emit):
> Make probabilities even for values in jump table
> according to number of cases handled.
>   (switch_decision_tree::compute_cases_per_edge): Pass
> argument to reset_out_edges_aux function.
>   (switch_decision_tree::analyze_switch_statement): Likewise.
>   * tree-switch-conversion.h (switch_decision_tree::reset_out_edges_aux):
> Make it static.
> ---
>  gcc/tree-switch-conversion.c | 40 ++--
>  gcc/tree-switch-conversion.h | 11 +-
>  2 files changed, 43 insertions(+), 8 deletions(-)
> 
OK.
jeff

Re: [PATCH] convert braced initializers to strings (PR 71625)

[Jeff Law]

On 08/17/2018 04:26 PM, Bernd Edlinger wrote:
> Hi everybody,
> 
> On 08/16/18 08:36, Bernd Edlinger wrote:
>> Jeff Law wrote:
>>> I wonder if the change to how we set up the initializers is ultimately
>>> changing the section those go into and ultimately causing an overflow of
>>> the .sdata section.
>>
>>
>> Yes, that is definitely the case.
>> Due to the -fmerge-all-constants option used
>> named arrays with brace initializer look like string initializers
>> and can go into the merge section if there are no embedded nul chars.
>> But the string constants can now be huge.
>>
>> See my other patch about string merging:
>> [PATCH] Handle not explicitly zero terminated strings in merge sections
>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00481.html
>>
>>
>> Can this section overflow?
>>
> 
> 
> could someone try out if this (untested) patch fixes the issue?
It seems to in my limited testing.

jeff

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

[Jeff Law]

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.
> 
> There's a difference between explicitly not handling it and silently 
> passing a wrong value.
> 
>> In theory something like WCHAR_TYPE_SIZE / BITS_PER_UNIT probably does
>> the trick.   I'm a bit leery of using that though.  We don't use it
>> anywhere else within GCC AFAICT.
> 
> WCHAR_TYPE_SIZE is wrong because it doesn't account for flag_short_wchar.  
> As far as I can see only ada/gcc-interface/targtyps.c uses WCHAR_TYPE_SIZE 
> now.  TYPE_PRECISION (wchar_type_node) / BITS_PER_UNIT is what should be 
> used.
But that's specific to the c-family front-ends.

There's MODIFIED_WCHAR_TYPE which is ultimately used to build
wchar_type_node for the c-family front-ends.  Maybe we could construct
something from that.


jeff

Re: [PING] [PATCH] Fix wrong code with truncated string literals (PR 86711/86714)

[Jeff Law]

On 08/17/2018 03:14 AM, Bernd Edlinger wrote:
> Hi!
> 
> 
> After the other patch has been applied, I re-based this patch accordingly.
> 
> Except the mechanical changes, there are a few notable differences to the
> previous version:
> 
> In string_constant, I added a similar check for the STRING_CSTs
> because when callers don't use mem_size, they assume to be
> able to read "TREE_STRING_LENGTH (array)" bytes, but that is
> not always the case, for languages that don't always use
> zero-terminated strings, for instance hollerith strings in fortran.
> 
> --- gcc/expr.c  2018-08-17 05:32:57.332211963 +0200
> +++ gcc/expr.c  2018-08-16 23:08:23.544940795 +0200
> @@ -11372,6 +11372,9 @@ string_constant (tree arg, tree *ptr_off
>*ptr_offset = fold_convert (sizetype, offset);
>if (mem_size)
> *mem_size = TYPE_SIZE_UNIT (TREE_TYPE (array));
> +  else if (compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (array)),
> +TREE_STRING_LENGTH (array)) < 0)
> +   return NULL_TREE;
>return array;
>  }
> 
Yes.  I purposefully didn't include this change in its entirety as it
was dependent upon your earlier patch.   Instead I twiddled your patch
so that it applied on the current trunk and didn't regress anything
while keeping the core concept you were introducing.


I'm also confident that change breaks one or more tests in the
testsuite.  I'm not sure why you didn't see the regression.   But I
certainly saw it with the variant shown above.

Anyway, the basic idea was to carve out the basic concept of your patch
to allow callers to specify how to count without regressing the trunk.

That allows us to carve out an issue, resolve it and move on.  The
interdependent and conflicting patches are a nightmare to sort out.

> 
> The range check in c_getstr was refined as well:
> 
> This I added, because vla arrays can be initialized with string constants,
> especially since the 71625 patch was installed:
> In this case we end up with mem_size that fails to be constant.
> 
> 
> @@ -14606,25 +14603,17 @@ c_getstr (tree src, unsigned HOST_WIDE_I
> offset = tree_to_uhwi (offset_node);
>  }
> 
> +  if (!tree_fits_uhwi_p (mem_size))
> +return NULL;
> +
>/* STRING_LENGTH is the size of the string literal, including any
>   embedded NULs.  STRING_SIZE is the size of the array the string
>   literal is stored in.  */
> 
> Also the rest of the string length checks are refined, to return
> actually zero-terminated single byte strings when strlen is not given,
> and return something not necessarily zero-terminated which is
> suitable for memxxx-functions otherwise.

> 
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
Not yet.  There's a lot to go over here and I haven't finished reviewing
all the discussions around 86711/86714.


Jeff

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

[Jeff Law]

On 08/17/2018 09:43 PM, Paul Hua wrote:
> Hi Qing:
> 
>>
>> the change has been committed as:
>> https://gcc.gnu.org/viewcvs/gcc?view=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.
Ugh.  :(  There's probably other targets that are going to have similar
properties.  I'm not really sure how to write a suitable assembler test
for this.

Maybe we just need a different search result for MIPS (and potentially
other targets).

jeff

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

[Jeff Law]

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

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

 characters btw.

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

 char x[2] = "1";

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

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

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

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

>>>
>>> But at other places we look at the type of the string_cst, don't we?
>>> Shouldn't those be the same?
>>
>> I think most (all?) places look at TREE_TYPE (TREE_TYPE (string))
>> only.  I'm not aware of users of the array domain of the array type
>> of a string - but I'm far from knowing GCC inside-out ;)
>>
> 
> Yes, I know, that happens to me as well on the first day after my holidays ;)
I'm not aware of anything using the array domain of the array type of a
string.  But there's still a lot of old code in tree.c and expr.c.  Who
knows what would be found with a thorough audit.

jeff

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

[Paul Hua]

Hi Qing:

>
> the change has been committed as:
> https://gcc.gnu.org/viewcvs/gcc?view=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

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

[Jeff Law]

On 08/17/2018 06:13 AM, Bernd Edlinger wrote:
>>
> 
> Yes, there is a JIT code gen bug, that was caught by the assertion in my
> patch:
> 
> [PATCH] Fix not properly nul-terminated string constants in JIT
> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00370.html
THanks for the reference.  I didn't have it in my queue.

> 
> 
> That is a simple bug, which means, that the JIT FE was never able to
> assemble strings > 200 character length.
> 
> Fixing a bug like that makes me I wonder, if there is any test coverage
> for JIT except the gcc test suite itself.
Nothing I'm aware of except the gcc testsuite itself -- it was more of a
research project to find out what we'd need to fix in GCC if we ever
wanted to be able to use it as a JIT.

Jeff

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

[Jeff Law]

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.

As we've discussed, the C/GIMPLE semantic issues mean we can not rely on
types.  One could use that as an argument that the check is
fundamentally wrong, but in the case where things go screwy here we
always return NULL indicating we do not know the length. So it's the
safe thing to do.

On the other hand one could argue that the test is a canary that
something unexpected has happened, either in terms of the passed in
size, or the types of the argument.  For the case above, we miss the
warning.  Missing a warning is less worrisome than generating wrong code.

Another alternative (and I hate to suggest it) is to indicate via an
argument how the result of c_strlen is used.  ie, is it used just for
warnings or is it used for opt/codegen.  For the former we wouldn't use
the test.  For the latter we would.  That ultimately gets the warning
you want, but also keeps us safe for codegen/opt purposes.

Jeff

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

[Jeff Law]

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.
That's a different issue than what Martin is talking about, but is the
same as what Joseph is asking us to fix.

This is well outside my area of expertise, but I believe Windows has 16
bit wchar_t, so these tests probably aren't working correctly on
cygwin/mingw.

Jeff

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

[Jeff Law]

On 08/17/2018 12:23 PM, 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.)
Interestingly enough that check failing was instrumental in debugging a
failing testcase when I was poking a bit at Bernd's patch.  I probably
would have found the issue without that check, but it was pretty obvious
as I stepped through c_strlen.

I don't have a strong opinion on the check (keep vs drop).  I do think
that passing in the size makes more sense than trying to determine it
within c_strlen.  The latter is going to stumble over the C vs GIMPLE
semantics issues we've discussed elsewhere.

Jeff

[PATCH] Adjust lazy macro definition

[Nathan Sidwell]

We provide a lazy macro definition hook, to deal with the FP macros 
where it is expensive to generate the string representation of the FP 
value.  The current hook is much more general than needed, but it's not 
clear that generality can be used because we don't document what macro 
state can be manipulated.  We create the macro early with a bogus FP 
token, then overwrite the identifier node making it look like a builtin 
node with an out-of-range index.  Upon first use we call the hook which 
checks the index value, and if ok updates with the saved macro after 
nadgering the token of interest with the correct string.


This patch makes things cleaner.  We don't overload the builtin index 
meaning, rather we add a lazy field to cpp_macro, and allow the user to 
control that.  There's plenty of spare space there, so this doesn't 
increase the macro size.


Rather than have the user's hook fish out the cpp_macro object, we 
provide the object directly, along with the lazy hook value for it to 
inspect.  It can then update the macro as desired and declare victory.


We already have _cpp_maybe_notify_macro_use that deals with lazy 
definition, it's simply a matter of adjusting that and using it in a few 
more place that are now easier.


Finally, the PCH machinery was invoking the lazy hook during PCH write 
out.  This seems fragile to me -- changing the memory state during 
serialization.  We don't need to do that, as the lazy machinery is all 
GTY marked.  This does mean we'll lazily define the PCH'd macro upon 
every first use after reading back, but that doesn't seem costly to me.


booted & tested on x86_64-linux.

nathan
--
Nathan Sidwell
2018-08-17  Nathan Sidwell  

	libcpp/
	* include/cpplib.h (struct cpp_callbacks): Replace
	user_builtin_macro with user_lazy_macro.
	(struct cpp_macro): add lazy field.
	(enum cpp_builtin_type): Remove BT_FIRST_USER, BT_LAST_USER.
	(cpp_define_lazily): Declare.
	* macro.c (enter_macro_context) Use _cpp_maybe_notify_macro_use.
	(warn_of_redefinition): Use cpp_builtin_macro_p, directly call
	user_lazy_macro hook.
	(_cpp_new_macro): Clear lazy field.
	(cpp_define_lazily): Define.
	(_cpp_notify_macro_use): Adjust lazy definition code.
	(cpp_macro_definition): No need to do lazy definition here.
	* pch.c (write_macdef, save_macros): Likewise.
	gcc/c-family/
	* c-cppbuiltin.c (struct lazy_hex_fp_value_struct): Remove macro
	field.
	(laxy_hex_fp_value_count): Make unsigned.
	(lazy_hex_fp_value): Provided with macro & lazy number.  Directly
	manipulate the macro.
	(builtin_defin_with_hex_fp_value): Adjust callback name, use
	cpp_define_lazily.

Index: gcc/c-family/c-cppbuiltin.c
===
--- gcc/c-family/c-cppbuiltin.c	(revision 263622)
+++ gcc/c-family/c-cppbuiltin.c	(working copy)
@@ -1570,7 +1570,6 @@ builtin_define_with_int_value (const cha
 struct GTY(()) lazy_hex_fp_value_struct
 {
   const char *hex_str;
-  cpp_macro *macro;
   machine_mode mode;
   int digits;
   const char *fp_suffix;
@@ -1583,36 +1582,35 @@ struct GTY(()) lazy_hex_fp_value_struct
 #define LAZY_HEX_FP_VALUES_CNT (4 * (3 + NUM_FLOATN_NX_TYPES))
 static GTY(()) struct lazy_hex_fp_value_struct
   lazy_hex_fp_values[LAZY_HEX_FP_VALUES_CNT];
-static GTY(()) int lazy_hex_fp_value_count;
+static GTY(()) unsigned lazy_hex_fp_value_count;
 
-static bool
-lazy_hex_fp_value (cpp_reader *pfile ATTRIBUTE_UNUSED,
-		   cpp_hashnode *node)
+static void
+lazy_hex_fp_value (cpp_reader *, cpp_macro *macro, unsigned num)
 {
   REAL_VALUE_TYPE real;
   char dec_str[64], buf1[256];
-  unsigned int idx;
-  if (node->value.builtin < BT_FIRST_USER
-  || (int) node->value.builtin >= BT_FIRST_USER + lazy_hex_fp_value_count)
-return false;
 
-  idx = node->value.builtin - BT_FIRST_USER;
-  real_from_string (, lazy_hex_fp_values[idx].hex_str);
+  gcc_checking_assert (num < lazy_hex_fp_value_count);
+
+  real_from_string (, lazy_hex_fp_values[num].hex_str);
   real_to_decimal_for_mode (dec_str, , sizeof (dec_str),
-			lazy_hex_fp_values[idx].digits, 0,
-			lazy_hex_fp_values[idx].mode);
+			lazy_hex_fp_values[num].digits, 0,
+			lazy_hex_fp_values[num].mode);
+
+  size_t len
+= sprintf (buf1, "%s%s", dec_str, lazy_hex_fp_values[num].fp_suffix);
+  gcc_assert (len < sizeof (buf1));
+  for (unsigned idx = 0; idx < macro->count; idx++)
+if (macro->exp.tokens[idx].type == CPP_NUMBER)
+  {
+	macro->exp.tokens[idx].val.str.len = len;
+	macro->exp.tokens[idx].val.str.text
+	  = (const unsigned char *) ggc_strdup (buf1);
+	return;
+  }
 
-  sprintf (buf1, "%s%s", dec_str, lazy_hex_fp_values[idx].fp_suffix);
-  node->flags &= ~(NODE_BUILTIN | NODE_USED);
-  node->value.macro = lazy_hex_fp_values[idx].macro;
-  for (idx = 0; idx < node->value.macro->count; idx++)
-if (node->value.macro->exp.tokens[idx].type == CPP_NUMBER)
-  break;
-  gcc_assert (idx < node->value.macro->count);
-  node->value.macro->exp.tokens[idx].val.str.len = 

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

[Bernd Edlinger]

On 08/17/18 22:17, 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);
>    }
> 
> There may be no other analyses that would benefit from this
> ability today but there easily could be.  There certainly
> are optimizations that depend on c_getstr() returning
> a pointer to the constant object regardless of its type
> (memchr being one of them).
> 

Yes, I agree.

Coincidentally that is exactly what in my follow-up patch implements.
See: https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01005.html

If you call c_getstr(x) you get a valid zero-terminated single-byte
string or nothing.

If you call c_getstr(x, ) you get a pointer to a memory
of the specified memsize, regardless of the underlying type.
and whether or not zero terminated.


Bernd.

Re: [PATCH] convert braced initializers to strings (PR 71625)

[Bernd Edlinger]

Hi everybody,

On 08/16/18 08:36, Bernd Edlinger wrote:
> Jeff Law wrote:
>> I wonder if the change to how we set up the initializers is ultimately
>> changing the section those go into and ultimately causing an overflow of
>> the .sdata section.
> 
> 
> Yes, that is definitely the case.
> Due to the -fmerge-all-constants option used
> named arrays with brace initializer look like string initializers
> and can go into the merge section if there are no embedded nul chars.
> But the string constants can now be huge.
> 
> See my other patch about string merging:
> [PATCH] Handle not explicitly zero terminated strings in merge sections
> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00481.html
> 
> 
> Can this section overflow?
> 


could someone try out if this (untested) patch fixes the issue?


Thanks,
Bernd.


2018-08-18  Bernd Edlinger  

	* expmed.c (simple_mem_bitfield_p): Do shift right signed.
	* config/alpha/alpha.h (CONSTANT_ADDRESS_P): Avoid signed
	integer overflow.

Index: gcc/config/alpha/alpha.h
===
--- gcc/config/alpha/alpha.h	(revision 263611)
+++ gcc/config/alpha/alpha.h	(working copy)
@@ -678,7 +678,7 @@ enum reg_class {
 
 #define CONSTANT_ADDRESS_P(X)   \
   (CONST_INT_P (X)		\
-   && (unsigned HOST_WIDE_INT) (INTVAL (X) + 0x8000) < 0x1)
+   && (UINTVAL (X) + 0x8000) < 0x1)
 
 /* The macros REG_OK_FOR..._P assume that the arg is a REG rtx
and check its validity for a certain class.
Index: gcc/expmed.c
===
--- gcc/expmed.c	(revision 263611)
+++ gcc/expmed.c	(working copy)
@@ -579,8 +579,12 @@ static bool
 simple_mem_bitfield_p (rtx op0, poly_uint64 bitsize, poly_uint64 bitnum,
 		   machine_mode mode, poly_uint64 *bytenum)
 {
+  poly_int64 ibit = bitnum;
+  poly_int64 ibyte;
+  if (!multiple_p (ibit, BITS_PER_UNIT, ))
+return false;
+  *bytenum = ibyte;
   return (MEM_P (op0)
-	  && multiple_p (bitnum, BITS_PER_UNIT, bytenum)
 	  && known_eq (bitsize, GET_MODE_BITSIZE (mode))
 	  && (!targetm.slow_unaligned_access (mode, MEM_ALIGN (op0))
 	  || (multiple_p (bitnum, GET_MODE_ALIGNMENT (mode))

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

[Segher Boessenkool]

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.

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

>   * 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! ].


Segher

[committed]: C++: -Wwrite-strings: use location of string constant

[David Malcolm]

Consider:

extern int callee (const char *one, char *two, const char *three);

int test ()
{
  return callee ("first", "second", "third");
}

for which -Wwrite-strings was emitting:

Wwrite-strings.C: In function 'int test()':
Wwrite-strings.C:10:44: warning: ISO C++ forbids converting a string constant 
to 'char*' [-Wwrite-strings]
10 |   return callee ("first", "second", "third");
   |^

This patch fixes the warning so that it underlines the pertinent argument
at the callsite:

Wwrite-strings.C: In function 'int test()':
Wwrite-strings.C:10:27: warning: ISO C++ forbids converting a string constant 
to 'char*' [-Wwrite-strings]
10 |   return callee ("first", "second", "third");
   |   ^~~~

Ideally we ought to also issue a "note" highlighting the pertinent
parameter within the decl, but that's not readily available, so I'm
saving it for another patch.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

Committed to trunk as r263635.

gcc/cp/ChangeLog:
* typeck.c (string_conv_p): Extract location from EXP and use it
in preference to input_location when issuing warnings.

gcc/testsuite/ChangeLog:
* g++.dg/conversion/Wwrite-strings.C: New test.
---
 gcc/cp/typeck.c  | 10 ++
 gcc/testsuite/g++.dg/conversion/Wwrite-strings.C | 24 
 2 files changed, 30 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/conversion/Wwrite-strings.C

diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 64b3d58..8c13ae9 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -2208,6 +2208,8 @@ string_conv_p (const_tree totype, const_tree exp, int 
warn)
   && !same_type_p (t, wchar_type_node))
 return 0;
 
+  location_t loc = EXPR_LOC_OR_LOC (exp, input_location);
+
   STRIP_ANY_LOCATION_WRAPPER (exp);
 
   if (TREE_CODE (exp) == STRING_CST)
@@ -2230,13 +2232,13 @@ string_conv_p (const_tree totype, const_tree exp, int 
warn)
   if (warn)
 {
   if (cxx_dialect >= cxx11)
-   pedwarn (input_location, OPT_Wwrite_strings,
+   pedwarn (loc, OPT_Wwrite_strings,
 "ISO C++ forbids converting a string constant to %qT",
 totype);
   else
-   warning (OPT_Wwrite_strings,
-"deprecated conversion from string constant to %qT",
-totype);
+   warning_at (loc, OPT_Wwrite_strings,
+   "deprecated conversion from string constant to %qT",
+   totype);
 }
 
   return 1;
diff --git a/gcc/testsuite/g++.dg/conversion/Wwrite-strings.C 
b/gcc/testsuite/g++.dg/conversion/Wwrite-strings.C
new file mode 100644
index 000..f6dbb15
--- /dev/null
+++ b/gcc/testsuite/g++.dg/conversion/Wwrite-strings.C
@@ -0,0 +1,24 @@
+// { dg-options "-fdiagnostics-show-caret" }
+
+/* Verify that -Wwrite-strings underlines the string literal in question.  */
+
+extern int callee (const char *one, char *two, const char *three);
+
+int test_1 ()
+{
+  return callee ("first", "second", "third"); // { dg-warning "string constant 
to 'char\\*'" }
+  /* { dg-begin-multiline-output "" }
+   return callee ("first", "second", "third");
+   ^~~~
+ { dg-end-multiline-output "" } */
+  // TODO: underline the pertinent param in the decl of callee
+}
+
+char *test_2 (void)
+{
+  return "foo"; // { dg-warning "string constant to 'char\\*'" }
+  /* { dg-begin-multiline-output "" }
+   return "foo";
+  ^
+ { dg-end-multiline-output "" } */
+}
-- 
1.8.5.3

Re: [PATCH,rs6000] Add builtins for accessing the FPSCR

[Segher Boessenkool]

Hi Carl,

On Fri, Aug 17, 2018 at 11:46:06AM -0700, Carl Love wrote:
> > In addition to listing
> > the builtin, I added a C style comment to describe the builtin a
> > little.  I don't see any of the other builtins documented like this. 
> > But I felt some explanation of the builtins were
> > helpful.  Suggestions
> > on a better way to add the comments on the builtins would be
> > appreciated.

I think this is fine.

>   * config/rs6000/rs6000-builtin.def: Add definitions for __builtin_mffsl,
>   __builtin_mtfsb0, __builtin_mtfsb1, __builtin_set_fpscr_rn,
>   __builtin_set_fpscr_drn.

* config/rs6000/rs6000-builtin.def (__builtin_mffsl): New.
(__builtin_mtfsb0): New.
(__builtin_mtfsb1): New.
(__builtin_set_fpscr_rn): New.
(__builtin_set_fpscr_drn): New.

or

* config/rs6000/rs6000-builtin.def (__builtin_mffsl, __builtin_mtfsb0,
__builtin_mtfsb1, __builtin_set_fpscr_rn, __builtin_set_fpscr_drn): New.

>   * config/rs6000.c: Add functions rs6000_expand_mtfsb0_mtfsb1_builtin,
>   rs6000_expand_set_fpscr_rn_builtin, rs6000_expand_set_fpscr_drn_builtin.

Same here (and further on).

>   Add case statement entries for the new builtins.

To what function(s)?

>   * testsuite/gcc.target/powerpc/test_mffsl-p9.c: New file.
>   * testsuite/gcc.target/powerpc/test_fpscr_builtins.c: New file.
>   * testsuite/gcc.target/powerpc/test_fpscr_builtins_error.c: New file.

testsuite/ has its own changelog.  Entries in there do not include
"testsuite/".

> --- a/gcc/config/rs6000/rs6000-builtin.def
> +++ b/gcc/config/rs6000/rs6000-builtin.def
> @@ -2486,11 +2486,34 @@ BU_SPECIAL_X (RS6000_BUILTIN_MFTB, 
> "__builtin_ppc_mftb",
>  BU_SPECIAL_X (RS6000_BUILTIN_MFFS, "__builtin_mffs",
> RS6000_BTM_ALWAYS, RS6000_BTC_MISC)
>  
> +BU_SPECIAL_X (RS6000_BUILTIN_MFFSL, "__builtin_mffsl",
> +   RS6000_BTM_ALWAYS, RS6000_BTC_MISC)

Should this be RS6000_BTM_MISC_P9 (or similar) instead?  Same for the other
ISA 3.0 ops.

> +static rtx
> +rs6000_expand_mtfsb0_mtfsb1_builtin (enum insn_code icode, tree exp)

I'd call this rs6000_expand_mtfsb_builtin, but please use which you think
is clearest.

> +  /* Only allow bit numbers 0 to 31.  */
> +  if (GET_CODE (op0) != CONST_INT || INTVAL (op0) < 0 || INTVAL (op0) > 31)

if (!u5bit_cint_operand (op0, VOIDmode))

should do the trick I think.

> +{
> +   error ("Argument must be a constant between 0 and 31.");
> +   return const0_rtx;
> + }
> +
> +  if (! (*insn_data[icode].operand[0].predicate) (op0, mode0))
> +op0 = copy_to_mode_reg (mode0, op0);

Is this correct?  It must be a constant integer already, and if it fails
copying it into a register is surely not the right thing to do.

> +  /* If the argument is a constant, check the range. Agrument can only be a
> + 2-bit value.  Unfortunately, can't check the range of the value at
> + compile time if the argument is a variable.
> +  */
> +  if (GET_CODE (op0) == CONST_INT && (INTVAL (op0) < 0 || INTVAL (op0) > 3))

const_0_to_3_operand

> +/* Builtin not supported on this processor.  */
> +return 0;
> +
> +  /* If we got invalid arguments bail out before generating bad rtl.  */
> +  if (arg0 == error_mark_node)
> +return const0_rtx;
> +
> +  /* If the argument is a constant, check the range. Agrument can only be a
> + 3-bit value.  Unfortunately, can't check the range of the value at
> + compile time if the argument is a variable.
> +  */
> +  if (GET_CODE (op0) == CONST_INT && (INTVAL (op0) < 0 || INTVAL (op0) > 7))

(Typo, "argument").  const_0_to_7_operand or u3bit_cint_operand (both exist,
and they are identical.  Hrm.)

>  
> @@ -16370,6 +16497,30 @@ rs6000_init_builtins (void)
>ftype = build_function_type_list (double_type_node, NULL_TREE);
>def_builtin ("__builtin_mffs", ftype, RS6000_BUILTIN_MFFS);
>  
> +  ftype = build_function_type_list (double_type_node, NULL_TREE);
> +  def_builtin ("__builtin_mffsl", ftype, RS6000_BUILTIN_MFFSL);
> +
> +  ftype = build_function_type_list (void_type_node,
> + intSI_type_node,
> + NULL_TREE);
> +
> +  def_builtin ("__builtin_mtfsb0", ftype, RS6000_BUILTIN_MTFSB0_SI);

No blank line between ftype and def_builtin please?

> +(define_insn "rs6000_mtfsb0_si"

Why the _si?  Won't just rs6000_mtfsb0 do?

> + [(use (match_operand:SI 0 "short_cint_operand" "n"))
> +  (unspec_volatile:SI [(const_int 0)] UNSPECV_MTFSFB0)]

UNSPECV_MTFSB0 please.

operands[0] should be an argument of the unspec... so something like

(define_insn "rs6000_mtfsb0"
  [(unspec_volatile [(match_operand:SI 0 "u5bit_cint_operand" "n")]
UNSPECV_MTFSB0)]
  "TARGET_HARD_FLOAT"
  "mtfsb0 %0")

(and you should set the "type" attribute to something useful, ideally).

> +(define_insn "rs6000_mffscrn"
> +  [(set (match_operand:DF 0 "gpc_reg_operand" "=d")
> +   (unspec_volatile:DF [(const_int 

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

[Tom de Vries]

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.

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.
Otherwise, add a DW_AT_location to the referenced die.

---
 gcc/dwarf2out.c | 32 
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 9ed473088e7..e1dccb42823 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -20598,7 +20598,7 @@ static void
 add_scalar_info (dw_die_ref die, enum dwarf_attribute attr, tree value,
 int forms, struct loc_descr_context *context)
 {
-  dw_die_ref context_die, decl_die;
+  dw_die_ref context_die, decl_die = NULL;
   dw_loc_list_ref list;
   bool strip_conversions = true;
   bool placeholder_seen = false;
@@ -20675,7 +20675,7 @@ add_scalar_info (dw_die_ref die, enum dwarf_attribute 
attr, tree value,
 
   if (decl != NULL_TREE)
{
- dw_die_ref decl_die = lookup_decl_die (decl);
+ decl_die = lookup_decl_die (decl);
 
  /* ??? Can this happen, or should the variable have been bound
 first?  Probably it can, since I imagine that we try to create
@@ -20684,8 +20684,12 @@ add_scalar_info (dw_die_ref die, enum dwarf_attribute 
attr, tree value,
 later parameter.  */
  if (decl_die != NULL)
{
- add_AT_die_ref (die, attr, decl_die);
- return;
+ if (get_AT (decl_die, DW_AT_location)
+ || get_AT (decl_die, DW_AT_const_value))
+   {
+ add_AT_die_ref (die, attr, decl_die);
+ return;
+   }
}
}
 }
@@ -20729,15 +20733,19 @@ add_scalar_info (dw_die_ref die, enum dwarf_attribute 
attr, tree value,
   || placeholder_seen)
 return;
 
-  if (current_function_decl == 0)
-context_die = comp_unit_die ();
-  else
-context_die = lookup_decl_die (current_function_decl);
+  if (!decl_die)
+{
+  if (current_function_decl == 0)
+   context_die = comp_unit_die ();
+  else
+   context_die = lookup_decl_die (current_function_decl);
+
+  decl_die = new_die (DW_TAG_variable, context_die, value);
+  add_AT_flag (decl_die, DW_AT_artificial, 1);
+  add_type_attribute (decl_die, TREE_TYPE (value), TYPE_QUAL_CONST, false,
+ context_die);
+}
 
-  decl_die = new_die (DW_TAG_variable, context_die, value);
-  add_AT_flag (decl_die, DW_AT_artificial, 1);
-  add_type_attribute (decl_die, TREE_TYPE (value), 

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

[Martin Sebor]

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);
  }

There may be no other analyses that would benefit from this
ability today but there easily could be.  There certainly
are optimizations that depend on c_getstr() returning
a pointer to the constant object regardless of its type
(memchr being one of them).

Martin

Re: [PATCH] RFC: remove std::tuple partial specialization

[Jonathan Wakely]

On 17/08/18 22:46 +0300, Ville Voutilainen wrote:

On 17 August 2018 at 22:29, Jonathan Wakely  wrote:

That was added by https://gcc.gnu.org/ml/libstdc++/2016-12/msg00122.html
but I see no justification for that in the standard (and neither
libc++ nor MSFTL does anything special here, so they fail the test
too).

Ville, I'm no longer convinced by your rationale. Why is that


You mean the part where I said "the variadic constructor doesn't run
into this problem"?


Yes. Either we shouldn't constrain our 2-tuple partial specialization,
or the standard should require it for the sizeof...(Types)==2 case.


I may have been wrong about the variadic.


On 17/08/18 22:47 +0300, Ville Voutilainen wrote:

So yes, I strongly think we should report a defect.


OK, I'll do that.

Re: [PATCH] RFC: remove std::tuple partial specialization

[Ville Voutilainen]

On 17 August 2018 at 22:46, Ville Voutilainen
 wrote:
>>
>> If we think the test is right, we should report a defect. Either way,
>> I think this patch would be a nice simplification. We can either fix
>> (or just remove) the test, or constrain the primary template.
>
> I think the test is reasonable to the point of being obvious. If you
> pass an allocator_arg
> and an allocator, the intent is that uses-allocator construction is
> always used, not that
> it's used unless the element type is constructible from allocator_arg
> and an allocator.
> While the explicitness and presence of some tuple constructors depends
> on the properties
> of the element types, the semantics of the allocator constructors
> should not depend on them
> to decide whether to do uses-allocator construction or not - that
> would be a vector again.

So yes, I strongly think we should report a defect.

Re: [PATCH] RFC: remove std::tuple partial specialization

[Ville Voutilainen]

On 17 August 2018 at 22:29, Jonathan Wakely  wrote:
> That was added by https://gcc.gnu.org/ml/libstdc++/2016-12/msg00122.html
> but I see no justification for that in the standard (and neither
> libc++ nor MSFTL does anything special here, so they fail the test
> too).
>
> Ville, I'm no longer convinced by your rationale. Why is that

You mean the part where I said "the variadic constructor doesn't run
into this problem"?
I may have been wrong about the variadic.

> constraint on the 2-tuple partial specialization when the standard
> doesn't say anything like "if sizeof...(Types)==2 this constructor
> shall not participate in overload resolution unless decay_t is
> not allocator_arg_t" for the tuple(_UTypes&&...) constructor?
>
> As far as I can tell, the standard says that the test is wrong.
>
> If we think the test is right, we should report a defect. Either way,
> I think this patch would be a nice simplification. We can either fix
> (or just remove) the test, or constrain the primary template.

I think the test is reasonable to the point of being obvious. If you
pass an allocator_arg
and an allocator, the intent is that uses-allocator construction is
always used, not that
it's used unless the element type is constructible from allocator_arg
and an allocator.
While the explicitness and presence of some tuple constructors depends
on the properties
of the element types, the semantics of the allocator constructors
should not depend on them
to decide whether to do uses-allocator construction or not - that
would be a vector again.

[PATCH] doc: Use @ref, not @xref, in the middle of a sentence

[Segher Boessenkool]

makeinfo warns about this.  Obvious, tested, committing.


Segher


2018-08-17  Segher Boessenkool  

* doc/md.texi (Patterns): Use @ref instead of @xref in the middle of
a sentence.

---
 gcc/doc/md.texi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi
index 02f9e1e..fd8b9d2 100644
--- a/gcc/doc/md.texi
+++ b/gcc/doc/md.texi
@@ -144,7 +144,7 @@ a nameless pattern for all other purposes.  Names beginning 
with the
 
 The name may also have the form @samp{@@@var{n}}.  This has the same
 effect as a name @samp{@var{n}}, but in addition tells the compiler to
-generate further helper functions; see @xref{Parameterized Names} for details.
+generate further helper functions; see @ref{Parameterized Names} for details.
 
 @item
 The @dfn{RTL template}: This is a vector of incomplete RTL expressions
-- 
1.8.3.1

Re: [PATCH v2 2/4] libgcc: add crt{begin,end} for powerpc-wrs-vxworks target

[Olivier Hainque]

Hi Rasmus,

> On 16 Aug 2018, at 11:30, Rasmus Villemoes  wrote:
> 
> On 2018-06-28 10:43, Rasmus Villemoes wrote:
>> In order to allow ZCX on VxWorks, we need the frame_dummy function to do
>> the register_frame_info(). So make sure crtbegin.o and crtend.o are
>> available for use with a custom spec file.
> 
> Hi Olivier
> 
> Can I also have your explicit ok for patch 2/4 (I'll fix the changelog
> as for the other patches)?

Yes, this is fine.

We're working on a possible replacement of those crt
files for VxWorks. Just not quite ready yet and no reason
to block the current ones in the interim.

Olivier

[PATCH] RFC: remove std::tuple partial specialization

[Jonathan Wakely]

While fixing PR 86963 I realised we can get rid of the 2-tuple partial
specialization, and just add the relevant constructors and assignment
operators to the primary template. They're all constrained anyway, so
they won't be available except when sizeof...(_Elements) == 2.

This patch also removes the recursive evaluation of exception
specifications on the assignment operators and tuple::swap members,
just defining them on std::tuple without depending on the base
classes.

BUT it causes:

FAIL: 20_util/tuple/cons/allocator_with_any.cc execution test
/home/jwakely/src/gcc/gcc/libstdc++-v3/testsuite/20_util/tuple/cons/allocator_with_any.cc:35:
 void test01(): Assertion 'std::get<0>(t).empty()' failed.

where that test does:

   std::tuple t(std::allocator_arg,
   std::allocator{});
   VERIFY(std::get<0>(t).empty());
   VERIFY(std::get<1>(t).empty());

That's because the partial specialization had a special case for
allocator_arg_t:

-  template()
-  && _TMC::template
-_ImplicitlyMoveConvertibleTuple<_U1, _U2>()
- && !is_same::type,
- allocator_arg_t>::value,
-   bool>::type = true>
-constexpr tuple(_U1&& __a1, _U2&& __a2)
-   : _Inherited(std::forward<_U1>(__a1), std::forward<_U2>(__a2)) { }

That was added by https://gcc.gnu.org/ml/libstdc++/2016-12/msg00122.html
but I see no justification for that in the standard (and neither
libc++ nor MSFTL does anything special here, so they fail the test
too).

Ville, I'm no longer convinced by your rationale. Why is that
constraint on the 2-tuple partial specialization when the standard
doesn't say anything like "if sizeof...(Types)==2 this constructor
shall not participate in overload resolution unless decay_t is
not allocator_arg_t" for the tuple(_UTypes&&...) constructor?

As far as I can tell, the standard says that the test is wrong.

If we think the test is right, we should report a defect. Either way,
I think this patch would be a nice simplification. We can either fix
(or just remove) the test, or constrain the primary template.


commit 6fea64cd9f546e587c7212c7b95fde0b3005f936
Author: Jonathan Wakely 
Date:   Fri Aug 17 19:07:36 2018 +0100

Remove std::tuple partial specialization

diff --git a/libstdc++-v3/include/std/tuple b/libstdc++-v3/include/std/tuple
index 955b853066f..7cf3184b4aa 100644
--- a/libstdc++-v3/include/std/tuple
+++ b/libstdc++-v3/include/std/tuple
@@ -298,8 +298,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   _Tuple_impl&
   operator=(_Tuple_impl&& __in)
-  noexcept(__and_,
-	  is_nothrow_move_assignable<_Inherited>>::value)
   {
 	_M_head(*this) = std::forward<_Head>(_M_head(__in));
 	_M_tail(*this) = std::move(_M_tail(__in));
@@ -329,8 +327,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 protected:
   void
   _M_swap(_Tuple_impl& __in)
-  noexcept(__is_nothrow_swappable<_Head>::value
-   && noexcept(_M_tail(__in)._M_swap(_M_tail(__in
   {
 	using std::swap;
 	swap(_M_head(*this), _M_head(__in));
@@ -429,7 +425,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   _Tuple_impl&
   operator=(_Tuple_impl&& __in)
-  noexcept(is_nothrow_move_assignable<_Head>::value)
   {
 	_M_head(*this) = std::forward<_Head>(_M_head(__in));
 	return *this;
@@ -455,7 +450,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 protected:
   void
   _M_swap(_Tuple_impl& __in)
-  noexcept(__is_nothrow_swappable<_Head>::value)
   {
 	using std::swap;
 	swap(_M_head(*this), _M_head(__in));
@@ -502,6 +496,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
  __not_>
   >::value;
 }
+
 template
 static constexpr bool _NotSameTuple()
 {
@@ -544,6 +539,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 {
   return true;
 }
+
 template
 static constexpr bool _NotSameTuple()
 {
@@ -740,6 +736,39 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 explicit constexpr tuple(tuple<_UElements...>&& __in)
 : _Inherited(static_cast<_Tuple_impl<0, _UElements...>&&>(__in)) { }
 
+  // Shortcut for constraining the constructors taking pairs.
+  using _TP = _TC;
+
+  template()
+	&& _TP::template _ImplicitlyConvertibleTuple<_U1, _U2>(),
+	  bool> = true>
+	constexpr tuple(const pair<_U1, _U2>& __in)
+	: _Inherited(__in.first, __in.second) { }
+
+  template()
+	&& ! _TP::template _ImplicitlyConvertibleTuple<_U1, _U2>(),
+	  bool> = false>
+	explicit constexpr tuple(const pair<_U1, _U2>& __in)
+	: _Inherited(__in.first, __in.second) { }
+
+  template()
+	&& _TP::template _ImplicitlyMoveConvertibleTuple<_U1, _U2>(),
+	  bool> = true>
+	constexpr tuple(pair<_U1, _U2>&& __in)
+	: _Inherited(std::forward<_U1>(__in.first),
+		 std::forward<_U2>(__in.second)) { }
+
+  template()
+	&& ! _TP::template _ImplicitlyMoveConvertibleTuple<_U1, _U2>(),
+	  bool> = false>
+	explicit constexpr tuple(pair<_U1, _U2>&& __in)
+	

Re: [2/5] C-SKY port v3: Backend implementation

[Sandra Loosemore]

On 08/16/2018 11:45 PM, Jeff Law wrote:

On 08/05/2018 10:34 PM, Sandra Loosemore wrote:


csky-gcc-2.log


2018-08-05  Jojo  
Huibin Wang  
Sandra Loosemore  
Chung-Lin Tang  

C-SKY port: Backend implementation

gcc/
* config/csky/*: New.
* common/config/csky/*: New.


I did another once-over and I think this is fine for the trunk.  Make
sure to add a MAINTAINERS entry.


Thanks very much for the review!  I've committed everything, including 
both the MAINTAINERS update and the wwwdocs patch I posted a while back.


-Sandra

Re: [committed] diagnostics: add line numbers to source (PR other/84889)

[Andreas Schwab]

On Aug 17 2018, Manuel López-Ibáñez  wrote:

> In my Emacs, it jumps to column 8 (where the "a" is).

That's because there is no way to place it on column 1.

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] PR libstdc++/86963 Implement LWG 2729 constraints on tuple assignment

[Jonathan Wakely]

On 17/08/18 19:01 +0100, Jonathan Wakely wrote:

On 17/08/18 18:52 +0100, Jonathan Wakely wrote:

+  // The tag parameter ensures that in nested tuples each __tuple_base
+  // is a different type and can use the empty base-class optimisation.
+  template
+class __tuple_base


Specifically, this would fail if __tuple_base was not a class
template:

static_assert(sizeof(tuple>) == sizeof(int), "");

And also:

struct empty {};
static_assert(sizeof(tuple, tuple>) == 2, "");



In fact, it's just occurred to me that we don't really need the
__tuple_base class template at all. We can make _Tuple_impl
non-assignable (adding _M_assign members for the required
functionality). Then we don't need an extra base class.

Re: [PATCH,rs6000] Add builtins for accessing the FPSCR

[Carl Love]

GCC maintainers:

> In addition to listing
> the builtin, I added a C style comment to describe the builtin a
> little.  I don't see any of the other builtins documented like this. 
> But I felt some explanation of the builtins were
> helpful.  Suggestions
> on a better way to add the comments on the builtins would be
> appreciated.
> 

I spent some more time looking at the documentation file and decided my
first approach really isn't correct.  The needed comments should be
placed in a paragraph below where the list of builtins.  I updated the
patch with this change to the doc/extend.texi file.  

 Carl Love
---

gcc/ChangeLog:

2018-08-17  Carl Love  

* config/rs6000/rs6000-builtin.def: Add definitions for __builtin_mffsl,
__builtin_mtfsb0, __builtin_mtfsb1, __builtin_set_fpscr_rn,
__builtin_set_fpscr_drn.
* config/rs6000.c: Add functions rs6000_expand_mtfsb0_mtfsb1_builtin,
rs6000_expand_set_fpscr_rn_builtin, rs6000_expand_set_fpscr_drn_builtin.
Add case statement entries for the new builtins.
* config/rs6000.md: Add define_insn for rs6000_mtfsb0_si,
rs6000_mtfsb1_si, rs6000_mffscrn, rs6000_mffscdrn.
Add define_expand for rs6000_set_fpscr_rn and rs6000_set_fpscr_drn.
* doc/extend.texi: Add documentation for the builtins.

gcc/testsuite/ChangeLog:

2018-08-16  Carl Love  

* testsuite/gcc.target/powerpc/test_mffsl-p9.c: New file.
* testsuite/gcc.target/powerpc/test_fpscr_builtins.c: New file.
* testsuite/gcc.target/powerpc/test_fpscr_builtins_error.c: New file.
---
 gcc/config/rs6000/rs6000-builtin.def  |  23 ++
 gcc/config/rs6000/rs6000.c| 151 ++
 gcc/config/rs6000/rs6000.md   | 149 -
 gcc/doc/extend.texi   |  36 ++-
 .../gcc.target/powerpc/test_fpscr_builtins.c  | 282 ++
 .../powerpc/test_fpscr_builtins_error.c   |  26 ++
 .../gcc.target/powerpc/test_mffsl-p9.c|  36 +++
 7 files changed, 701 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/test_fpscr_builtins.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/test_fpscr_builtins_error.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/test_mffsl-p9.c

diff --git a/gcc/config/rs6000/rs6000-builtin.def 
b/gcc/config/rs6000/rs6000-builtin.def
index f79968154..a50236e77 100644
--- a/gcc/config/rs6000/rs6000-builtin.def
+++ b/gcc/config/rs6000/rs6000-builtin.def
@@ -2486,11 +2486,34 @@ BU_SPECIAL_X (RS6000_BUILTIN_MFTB, "__builtin_ppc_mftb",
 BU_SPECIAL_X (RS6000_BUILTIN_MFFS, "__builtin_mffs",
  RS6000_BTM_ALWAYS, RS6000_BTC_MISC)
 
+BU_SPECIAL_X (RS6000_BUILTIN_MFFSL, "__builtin_mffsl",
+ RS6000_BTM_ALWAYS, RS6000_BTC_MISC)
+
 RS6000_BUILTIN_X (RS6000_BUILTIN_MTFSF, "__builtin_mtfsf",
  RS6000_BTM_ALWAYS,
  RS6000_BTC_MISC | RS6000_BTC_UNARY | RS6000_BTC_VOID,
  CODE_FOR_rs6000_mtfsf)
 
+RS6000_BUILTIN_X (RS6000_BUILTIN_MTFSB0_SI, "__builtin_mtfsb0",
+ RS6000_BTM_ALWAYS,
+ RS6000_BTC_MISC | RS6000_BTC_UNARY,
+ CODE_FOR_rs6000_mtfsb0_si)
+
+RS6000_BUILTIN_X (RS6000_BUILTIN_MTFSB1_SI, "__builtin_mtfsb1",
+ RS6000_BTM_ALWAYS,
+ RS6000_BTC_MISC | RS6000_BTC_UNARY,
+ CODE_FOR_rs6000_mtfsb1_si)
+
+RS6000_BUILTIN_X (RS6000_BUILTIN_SET_FPSCR_RN, "__builtin_set_fpscr_rn",
+ RS6000_BTM_ALWAYS,
+ RS6000_BTC_MISC | RS6000_BTC_UNARY,
+ CODE_FOR_rs6000_set_fpscr_rn)
+
+RS6000_BUILTIN_X (RS6000_BUILTIN_SET_FPSCR_DRN, "__builtin_set_fpscr_drn",
+ RS6000_BTM_ALWAYS,
+ RS6000_BTC_MISC | RS6000_BTC_UNARY,
+ CODE_FOR_rs6000_set_fpscr_drn)
+
 BU_SPECIAL_X (RS6000_BUILTIN_CPU_INIT, "__builtin_cpu_init",
  RS6000_BTM_ALWAYS, RS6000_BTC_MISC)
 
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index aa707b255..7db9c10a9 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -13356,6 +13356,113 @@ rs6000_expand_mtfsf_builtin (enum insn_code icode, 
tree exp)
   return NULL_RTX;
 }
 
+static rtx
+rs6000_expand_mtfsb0_mtfsb1_builtin (enum insn_code icode, tree exp)
+{
+  rtx pat;
+  tree arg0 = CALL_EXPR_ARG (exp, 0);
+  rtx op0 = expand_normal (arg0);
+  machine_mode mode0 = insn_data[icode].operand[0].mode;
+
+  if (icode == CODE_FOR_nothing)
+/* Builtin not supported on this processor.  */
+return 0;
+
+  /* If we got invalid arguments bail out before generating bad rtl.  */
+  if (arg0 == error_mark_node)
+return const0_rtx;
+
+  /* Only allow bit numbers 0 to 31.  */
+  if (GET_CODE (op0) != CONST_INT || INTVAL (op0) < 0 || INTVAL (op0) > 31)
+{
+   error ("Argument must be a constant between 0 and 31.");
+   

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

[Bernd Edlinger]

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.

If the type is in fact the wrong, wide char type, we already
have a Wformat warning, therefore we should have the same
information here, that the Wformat warning has.

It can be something simple, like a global variable, that
is initialized somewhere in c-family to
TYPE_PRECISION (wchar_type_node) / BITS_PER_UNIT
and left at 0, for other languages.


Bernd.

Re: [PATCH] Minor optimisations in operator new(size_t, align_val_t)

[Marc Glisse]

On Fri, 17 Aug 2018, Jonathan Wakely wrote:


On 17/08/18 19:28 +0200, Marc Glisse wrote:

On Mon, 13 Aug 2018, Jonathan Wakely wrote:


Thanks to Lars for the suggestions.

* libsupc++/new_opa.cc (operator new(size_t, align_val_t)): Use
__is_pow2 to check for valid alignment. Avoid branching when rounding
size to multiple of alignment.

Tested x86_64-linux, committed to trunk.


Are you getting better code with __is_pow2 on many platforms? As far as I 
can tell from a quick look at the patch (I didn't actually test it, I could 
be completely off), this replaces (x&(x-1))==0 with popcount(x)==1. On a 
basic x86_64, popcount calls into libgcc, which doesn't seem so good. On a 
more recent x86_64 (BMI1), x&(x-1) is a single instruction that sets a flag 
when the result is 0, that's hard to beat.


Then shouldn't we do that in __ispow2?

Even better would be a peephole optimisation to turn
__builtin_popcount(x)==1 into that.


There is the complication of how to handle 0. For x=0, ispow2(x) is false 
while (x&(x-1))==0 is true. So the transformation corresponds to 
__builtin_popcount(x)<=1, and for ==1 we need more complications unless we 
somehow know that x cannot be 0.



Or was the goal to accept an alignment of 0, and not an optimization?


Accepting alignment of 0 isn't the goal :-)

std::ispow2 should be the best way to check if an unsigned integer is
a power of two, so I wanted to use that instead of manual bit
twiddling.


Makes sense. The best way to test this may not be the same if we know that 
the number cannot be 0, but I guess that if we don't find a better way it 
would be possible to use __builtin_constant_p to select between x&(x-1) 
and popcount==1. Using range information to enable the compiler 
transformation is also possible. Both may require an explicit if(align==0) 
in operator new (whether it replaces align with another value or calls 
__builtin_unreachable() probably doesn't matter).



I hope that check will go away soon,


In that case, please ignore my comments on the speed of the check.

--
Marc Glisse

Re: [PATCH] v2: Formatted printing for dump_* in the middle-end

[David Malcolm]

On Thu, 2018-08-16 at 22:08 -0600, Jeff Law wrote:
> On 08/02/2018 11:54 AM, David Malcolm wrote:
> > On Tue, 2018-07-31 at 19:56 +, Joseph Myers wrote:
> > > On Tue, 31 Jul 2018, David Malcolm wrote:
> > > 
> > > > I didn't exhaustively check every callsite to the changed
> > > > calls;
> > > > I'm
> > > > assuming that -Wformat during bootstrap has effectively checked
> > > > that
> > > > for me.  Though now I think about it, I note that we use
> > > > HOST_WIDE_INT_PRINT_DEC in many places: is this guaranteed to
> > > > be a
> > > > valid input to pp_format on all of our configurations?
> > > 
> > > HOST_WIDE_INT_PRINT_DEC should not be considered safe with
> > > pp_format 
> > > (although since r197049 may have effectively stopped using %I64
> > > on
> > > MinGW 
> > > hosts, I'm not sure if there are current cases where it won't
> > > work).  
> > > Rather, it is the job of pp_format to map the 'w' length
> > > specifier
> > > to 
> > > HOST_WIDE_INT_PRINT_DEC etc.
> > > 
> > > I think it clearly makes for cleaner code to limit use of 
> > > HOST_WIDE_INT_PRINT_* to as few places as possible and to prefer
> > > use
> > > of 
> > > internal printf-like functions that accept formats such as %wd
> > > where 
> > > possible.
> > 
> > Thanks.
> > 
> > I grepped the tree for every use of HOST_WIDE_INT_PRINT* and found
> > all that were within dump_printf[_loc] calls.  All such uses were
> > within files of the form "gcc/tree-vect*.c".
> > 
> > Here's an updated version of the patch (v2) which fixes those
> > callsites to use "%wd" or "%wu"; the dumpfile.c changes are as
> > per v1.
> > 
> > Changed in v2:
> > * rebased to after r263239 (which also touched c-format.c/h)
> > * avoid HOST_WIDE_INT_PRINT* within dump_printf* calls (in
> >   gcc/tree-vect*.c)
> > 
> > I didn't add test coverage for %wd and %wu, as pretty-print.c
> > already
> > has selftest coverage for these.
> > 
> > Richard's review of the v1 patch was:
> > "The patch is OK if C family maintainers agree on their parts."
> > so I'm looking for review of:
> > (a) the c-format.c changes (Joseph?), and
> > (b) the tree-vect*.c changes (Richard?  Joseph?)
> > 
> > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> > 
> > Is this patch OK for trunk?
> > 
> > Thanks
> > Dave
> > 
> > 
> > Blurb from v1, for reference:
> > 
> > This patch converts dump_print and dump_printf_loc from using
> > printf (and thus ATTRIBUTE_PRINTF) to using a new pretty-printer
> > based on pp_format, which supports formatting middle-end types.
> > 
> > In particular, the following codes are implemented (in addition
> > to the standard pretty_printer ones):
> > 
> >%E: gimple *:
> >Equivalent to: dump_gimple_expr (MSG_*, TDF_SLIM, stmt, 0)
> >%G: gimple *:
> >Equivalent to: dump_gimple_stmt (MSG_*, TDF_SLIM, stmt, 0)
> >%T: tree:
> >Equivalent to: dump_generic_expr (MSG_*, arg, TDF_SLIM).
> > 
> > Hence it becomes possible to convert e.g.:
> > 
> >   if (dump_enabled_p ())
> > {
> >   dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
> >"not vectorized: different sized vector "
> >"types in statement, ");
> >   dump_generic_expr (MSG_MISSED_OPTIMIZATION, TDF_SLIM,
> > vectype);
> >   dump_printf (MSG_MISSED_OPTIMIZATION, " and ");
> >   dump_generic_expr (MSG_MISSED_OPTIMIZATION, TDF_SLIM,
> > nunits_vectype);
> >   dump_printf (MSG_MISSED_OPTIMIZATION, "\n");
> > }
> > 
> > into a one-liner:
> > 
> >   if (dump_enabled_p ())
> > dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
> >  "not vectorized: different sized vector "
> >  "types in statement, %T and %T\n",
> >  vectype, nunits_vectype);
> > 
> > Unlike regular pretty-printers, this one captures optinfo_item
> > instances for the formatted chunks as appropriate, so that when
> > written out to a JSON optimization record, the relevant parts of
> > the message are labelled by type, and by source location (so that
> > e.g. %G is entirely equivalent to using dump_gimple_stmt).
> > 
> > dump_printf and dump_printf_loc become marked with
> > ATTRIBUTE_GCC_DUMP_PRINTF, which the patch also implements.
> > 
> > gcc/c-family/ChangeLog:
> > * c-format.c (enum format_type): Add
> > gcc_dump_printf_format_type.
> > (gcc_dump_printf_length_specs): New.
> > (gcc_dump_printf_flag_pairs): New.
> > (gcc_dump_printf_flag_specs): New.
> > (gcc_dump_printf_char_table): New.
> > (format_types_orig): Add entry for "gcc_dump_printf".
> > (init_dynamic_diag_info): Set up length_char_specs and
> > conversion_specs for gcc_dump_printf_format_type.
> > (handle_format_attribute): Handle gcc_dump_printf_format_type.
> > 
> > gcc/ChangeLog:
> > * dump-context.h: Include "dumpfile.h".
> > (dump_context::dump_printf_va): Convert final param from
> > va_list
> > to va_list *.  Convert from 

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

[Martin Sebor]

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

Martin



There's a difference between explicitly not handling it and silently
passing a wrong value.


In theory something like WCHAR_TYPE_SIZE / BITS_PER_UNIT probably does
the trick.   I'm a bit leery of using that though.  We don't use it
anywhere else within GCC AFAICT.


WCHAR_TYPE_SIZE is wrong because it doesn't account for flag_short_wchar.
As far as I can see only ada/gcc-interface/targtyps.c uses WCHAR_TYPE_SIZE
now.  TYPE_PRECISION (wchar_type_node) / BITS_PER_UNIT is what should be
used.

Re: [PATCH] PR libstdc++/86963 Implement LWG 2729 constraints on tuple assignment

[Jonathan Wakely]

On 17/08/18 18:52 +0100, Jonathan Wakely wrote:

+  // The tag parameter ensures that in nested tuples each __tuple_base
+  // is a different type and can use the empty base-class optimisation.
+  template
+class __tuple_base


Specifically, this would fail if __tuple_base was not a class
template:

static_assert(sizeof(tuple>) == sizeof(int), "");

And also:

struct empty {};
static_assert(sizeof(tuple, tuple>) == 2, "");

Re: [PATCH] Minor optimisations in operator new(size_t, align_val_t)

[Jonathan Wakely]

On 17/08/18 19:28 +0200, Marc Glisse wrote:

On Mon, 13 Aug 2018, Jonathan Wakely wrote:


Thanks to Lars for the suggestions.

* libsupc++/new_opa.cc (operator new(size_t, align_val_t)): Use
__is_pow2 to check for valid alignment. Avoid branching when rounding
size to multiple of alignment.

Tested x86_64-linux, committed to trunk.


Are you getting better code with __is_pow2 on many platforms? As far 
as I can tell from a quick look at the patch (I didn't actually test 
it, I could be completely off), this replaces (x&(x-1))==0 with 
popcount(x)==1. On a basic x86_64, popcount calls into libgcc, which 
doesn't seem so good. On a more recent x86_64 (BMI1), x&(x-1) is a 
single instruction that sets a flag when the result is 0, that's hard 
to beat.


Then shouldn't we do that in __ispow2?

Even better would be a peephole optimisation to turn
__builtin_popcount(x)==1 into that.


Or was the goal to accept an alignment of 0, and not an optimization?


Accepting alignment of 0 isn't the goal :-)

std::ispow2 should be the best way to check if an unsigned integer is
a power of two, so I wanted to use that instead of manual bit
twiddling.

I hope that check will go away soon, if the compiler starts checking
for valid alignments at the call site. (That won't catch all misuses,
as there could be calls with non-constants, but we can't make it
completely foolproof, some people just deserve to get UB!)


+  sz = (sz + align - 1) & ~(align - 1);


Note that gcc immediately replaces ~(align - 1) with -align. It does 
it even if you compute align-1 on the previous line and write 
(sz+X)&~X in the hope of sharing the subtraction.


The goal there was to replace the branch for the 'if' and just do the
adjustment unconditionally.

[PATCH] PR libstdc++/86963 Implement LWG 2729 constraints on tuple assignment

[Jonathan Wakely]

PR libstdc++/86963
* include/std/tuple (__tuple_base): New class template with deleted
copy assignment operator.
(tuple, tuple<_T1, _T2>): Derive from __tuple_base so that
implicit copy/move assignment operator will be deleted/suppressed.
(tuple::__assignable, tuple<_T1, _T2>::__assignable): New helper
functions for SFINAE constraints on assignment operators.
(tuple::__nothrow_assignable, tuple<_T1, _T2>::__nothrow_assignable):
New helper functions for exception specifications.
(tuple::operator=(const tuple&), tuple::operator=(tuple&&))
(tuple<_T1, _T2>::operator=(const tuple&))
(tuple<_T1, _T2>::operator=(tuple&&)): Change parameter types to
__nonesuch_no_braces when the operator should be defined implicitly.
Use __nothrow_assignable for exception specifications.
(tuple::operator=(const tuple<_UElements...>&))
(tuple::operator=(tuple<_UElements...>&&))
(tuple<_T1, _T2>::operator=(const tuple<_U1, _U2>&))
(tuple<_T1, _T2>::operator=(tuple<_U1, _U2>&&))
(tuple<_T1, _T2>::operator=(const pair<_U1, _U2>&))
(tuple<_T1, _T2>::operator=(pair<_U1, _U2>&&)): Constrain using
__assignable and use __nothrow_assignable for exception
specifications.
* python/libstdcxx/v6/printers.py (is_specialization_of): Accept
gdb.Type as first argument, instead of a string.
(StdTuplePrinter._iterator._is_nonempty_tuple): New method to check
tuple for expected structure.
(StdTuplePrinter._iterator.__init__): Use _is_nonempty_tuple.
* testsuite/20_util/tuple/dr2729.cc: New test.
* testsuite/20_util/tuple/element_access/get_neg.cc: Change dg-error
to dg-prune-output.

Tested x86_64-linux, committed to trunk.


commit 6f8a632c16f417ac0e0db1b2a02a27708702279d
Author: Jonathan Wakely 
Date:   Fri Aug 17 16:12:21 2018 +0100

PR libstdc++/86963 Implement LWG 2729 constraints on tuple assignment

PR libstdc++/86963
* include/std/tuple (__tuple_base): New class template with deleted
copy assignment operator.
(tuple, tuple<_T1, _T2>): Derive from __tuple_base so that
implicit copy/move assignment operator will be deleted/suppressed.
(tuple::__assignable, tuple<_T1, _T2>::__assignable): New helper
functions for SFINAE constraints on assignment operators.
(tuple::__nothrow_assignable, tuple<_T1, 
_T2>::__nothrow_assignable):
New helper functions for exception specifications.
(tuple::operator=(const tuple&), tuple::operator=(tuple&&))
(tuple<_T1, _T2>::operator=(const tuple&))
(tuple<_T1, _T2>::operator=(tuple&&)): Change parameter types to
__nonesuch_no_braces when the operator should be defined implicitly.
Use __nothrow_assignable for exception specifications.
(tuple::operator=(const tuple<_UElements...>&))
(tuple::operator=(tuple<_UElements...>&&))
(tuple<_T1, _T2>::operator=(const tuple<_U1, _U2>&))
(tuple<_T1, _T2>::operator=(tuple<_U1, _U2>&&))
(tuple<_T1, _T2>::operator=(const pair<_U1, _U2>&))
(tuple<_T1, _T2>::operator=(pair<_U1, _U2>&&)): Constrain using
__assignable and use __nothrow_assignable for exception
specifications.
* python/libstdcxx/v6/printers.py (is_specialization_of): Accept
gdb.Type as first argument, instead of a string.
(StdTuplePrinter._iterator._is_nonempty_tuple): New method to check
tuple for expected structure.
(StdTuplePrinter._iterator.__init__): Use _is_nonempty_tuple.
* testsuite/20_util/tuple/dr2729.cc: New test.
* testsuite/20_util/tuple/element_access/get_neg.cc: Change dg-error
to dg-prune-output.

diff --git a/libstdc++-v3/include/std/tuple b/libstdc++-v3/include/std/tuple
index dd7daf7f1cf..955b853066f 100644
--- a/libstdc++-v3/include/std/tuple
+++ b/libstdc++-v3/include/std/tuple
@@ -551,9 +551,23 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 }
   };
 
+  // The tag parameter ensures that in nested tuples each __tuple_base
+  // is a different type and can use the empty base-class optimisation.
+  template
+class __tuple_base
+{
+  template friend struct tuple;
+  __tuple_base() = default;
+  ~__tuple_base() = default;
+  __tuple_base(const __tuple_base&) = default;
+  __tuple_base& operator=(const __tuple_base&) = delete;
+};
+
   /// Primary class template, tuple
   template
-class tuple : public _Tuple_impl<0, _Elements...>
+class tuple
+: public _Tuple_impl<0, _Elements...>,
+  private __tuple_base>
 {
   typedef _Tuple_impl<0, _Elements...> _Inherited;
 
@@ -573,6 +587,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 }
   };
 
+  template
+   

Re: [committed] diagnostics: add line numbers to source (PR other/84889)

[Manuel López-Ibáñez]

On 17/08/18 17:50, Andreas Schwab wrote:

On Aug 17 2018, Manuel López-Ibáñez  wrote:


However, I see that GCC trunk still counts tabs as 1-column, probably
because emacs counts tabs as one column when interpreting column numbers
in the output of GCC.


That is not true.  Emacs is using screen columns by default for almost
20 years now (see compilation-error-screen-columns).


Maybe I didn't properly explain what I mean. I mean that Emacs counts a tab as 
a "GCC" column, whatever number of visual columns the tab is configured to be 
in Emacs. The following code (with a tab before 'a'):


/* ñ/* */
/* a/* */
a

With gcc -c test.c -Wcomment, it gives:

test.c:1:10: warning: "/*" within comment [-Wcomment]
 /* ñ/* */
  ^
test.c:2:9: warning: "/*" within comment [-Wcomment]
 /* a/* */
 ^
test.c:3:2: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ at end of 
input
  a
  ^

When the above appears in a compilation-mode buffer, I can click (or press 
enter) on each diagnostic and Emacs will jump exactly to what is pointed by the 
"^". That is, for Emacs, 3:2 does not mean line 3, *visual* column 2. In my 
Emacs, it jumps to column 8 (where the "a" is).


Changing the output to be 3:9 (if GCC starts interpreting tabs as 8 spaces as 
recommended by the GCS or to the value given by -ftabstop) will make Emacs jump 
to the wrong place.


This is independent of multi-byte characters. GCC is pointing to the wrong 
place when the line contains "ñ".


I hope the above is clearer,

Manuel.

Re: [PATCH] Handle not explicitly zero terminated strings in merge sections

[Bernd Edlinger]

Hi,

this is an update of the patch, which has just two Ada test cases
added for string merging which are based on Olivier's suggested test case.

Note that except the "Check STRING_CSTs in varasm.c" patch series,
there is now another patch needed to work around issues with 71625:

[PATCH] Call braced_list_to_string after array size is fixed
https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01015.html


Thanks
Bernd.


On 08/07/18 14:55, Bernd Edlinger wrote:
> Hi,
> 
> I realized that the previous version did not handle
> zero terminated Ada constants correctly as in
> 
> $ cat hello.adb
> procedure Hello is
>     procedure puts  (S : String) with Import, Convention => C;
>     X : String(1..8) := "abcdefg" & Ascii.Nul;
> begin
>    puts ("1234" & Ascii.Nul );
>    puts (X);
> end;
> 
> accidentally strings were doubly nul-terminated, because I forgot to
> handle merge string sections in assemble_constant_contents, and
> because get_constant_size increased the string literal by one.
> 
> Fixed, and added a positive test case for the merging non-zero terminated
> strings in C.
> 
> 
> Boot-strapped and regtested on x86_64-pc-linux-gnu.
> Is it OK for trunk (after pre-condition patches)?
> 
> 
> Thanks
> Bernd.
gcc:
2018-08-04  Bernd Edlinger  

	* varasm.c (output_constant): Add new parameter merge_strings.
	Make strings properly zero terminated in merge string sections.
	(mergeable_string_section): Don't fail if the last char is non-zero.
	(assemble_variable_contents): Handle merge string sections.
	(assemble_variable): Likewise.
	(assemble_constant_contents): Likewise.
	(output_constant_def_contents): Likewise.
	(get_constant_size): Remove special handling of STRING_CSTs.
	(redundant_nul_term_p): New helper function.

testsuite:
2018-08-04  Bernd Edlinger  

	* gnat.dg/string_merge1.adb: New test.
	* gnat.dg/string_merge2.adb: New test.
	* gcc.dg/merge-all-constants-1.c: Adjust test.
	* gcc.dg/merge-all-constants-2.c: New test.

Index: gcc/varasm.c
===
--- gcc/varasm.c	(revision 263072)
+++ gcc/varasm.c	(working copy)
@@ -111,7 +111,8 @@ static int compare_constant (const tree, const tre
 static void output_constant_def_contents (rtx);
 static void output_addressed_constants (tree);
 static unsigned HOST_WIDE_INT output_constant (tree, unsigned HOST_WIDE_INT,
-	   unsigned int, bool);
+	   unsigned int, bool,
+	   bool = false);
 static void globalize_decl (tree);
 static bool decl_readonly_section_1 (enum section_category);
 #ifdef BSS_SECTION_ASM_OP
@@ -827,7 +828,7 @@ mergeable_string_section (tree decl ATTRIBUTE_UNUS
 	  unit = GET_MODE_SIZE (mode);
 
 	  /* Check for embedded NUL characters.  */
-	  for (i = 0; i < len; i += unit)
+	  for (i = 0; i < len - unit; i += unit)
 	{
 	  for (j = 0; j < unit; j++)
 		if (str[i + j] != '\0')
@@ -2117,7 +2118,7 @@ assemble_noswitch_variable (tree decl, const char
 
 static void
 assemble_variable_contents (tree decl, const char *name,
-			bool dont_output_data)
+			bool dont_output_data, bool merge_strings = false)
 {
   /* Do any machine/system dependent processing of the object.  */
 #ifdef ASM_DECLARE_OBJECT_NAME
@@ -2140,7 +2141,7 @@ assemble_variable_contents (tree decl, const char
 	output_constant (DECL_INITIAL (decl),
 			 tree_to_uhwi (DECL_SIZE_UNIT (decl)),
 			 get_variable_align (decl),
-			 false);
+			 false, merge_strings);
   else
 	/* Leave space for it.  */
 	assemble_zeros (tree_to_uhwi (DECL_SIZE_UNIT (decl)));
@@ -2316,7 +2317,9 @@ assemble_variable (tree decl, int top_level ATTRIB
 	switch_to_section (sect);
   if (align > BITS_PER_UNIT)
 	ASM_OUTPUT_ALIGN (asm_out_file, floor_log2 (align / BITS_PER_UNIT));
-  assemble_variable_contents (decl, name, dont_output_data);
+  assemble_variable_contents (decl, name, dont_output_data,
+  sect->common.flags & SECTION_MERGE
+  && sect->common.flags & SECTION_STRINGS);
   if (asan_protected)
 	{
 	  unsigned HOST_WIDE_INT int size
@@ -3300,12 +3303,7 @@ get_constant_section (tree exp, unsigned int align
 static HOST_WIDE_INT
 get_constant_size (tree exp)
 {
-  HOST_WIDE_INT size;
-
-  size = int_size_in_bytes (TREE_TYPE (exp));
-  if (TREE_CODE (exp) == STRING_CST)
-size = MAX (TREE_STRING_LENGTH (exp), size);
-  return size;
+  return int_size_in_bytes (TREE_TYPE (exp));
 }
 
 /* Subroutine of output_constant_def:
@@ -3468,7 +3466,8 @@ maybe_output_constant_def_contents (struct constan
constant's alignment in bits.  */
 
 static void
-assemble_constant_contents (tree exp, const char *label, unsigned int align)
+assemble_constant_contents (tree exp, const char *label, unsigned int align,
+			bool merge_strings = false)
 {
   HOST_WIDE_INT size;
 
@@ -3478,7 +3477,7 @@ static void
   targetm.asm_out.declare_constant_name (asm_out_file, label, exp, size);
 
   /* Output the value of EXP.  */
-  output_constant (exp, size, align, false);
+  output_constant 

Re: [PATCH] Minor optimisations in operator new(size_t, align_val_t)

[Marc Glisse]

On Mon, 13 Aug 2018, Jonathan Wakely wrote:


Thanks to Lars for the suggestions.

* libsupc++/new_opa.cc (operator new(size_t, align_val_t)): Use
__is_pow2 to check for valid alignment. Avoid branching when rounding
size to multiple of alignment.

Tested x86_64-linux, committed to trunk.


Are you getting better code with __is_pow2 on many platforms? As far as I 
can tell from a quick look at the patch (I didn't actually test it, I 
could be completely off), this replaces (x&(x-1))==0 with popcount(x)==1. 
On a basic x86_64, popcount calls into libgcc, which doesn't seem so good. 
On a more recent x86_64 (BMI1), x&(x-1) is a single instruction that sets 
a flag when the result is 0, that's hard to beat.


Or was the goal to accept an alignment of 0, and not an optimization?



+  sz = (sz + align - 1) & ~(align - 1);


Note that gcc immediately replaces ~(align - 1) with -align. It does it 
even if you compute align-1 on the previous line and write (sz+X)&~X in 
the hope of sharing the subtraction.


--
Marc Glisse

Re: [PATCH] Fix poly types after PR tree-optimization/71625 strlen optimization

[Kyrill Tkachov]

Hi Szabolcs,

On 17/08/18 18:18, Szabolcs Nagy wrote:

On 15/08/18 16:51, Martin Sebor wrote:
> On 08/15/2018 04:28 AM, James Greenhalgh wrote:
>> On Tue, Aug 14, 2018 at 09:34:08PM -0500, Martin Sebor wrote:
>>> On 08/14/2018 09:24 AM, Martin Sebor wrote:
 On 08/14/2018 09:08 AM, Martin Sebor wrote:
 --- gcc/config/aarch64/aarch64-builtins.c(revision 263537)
 +++ gcc/config/aarch64/aarch64-builtins.c(working copy)
 @@ -643,6 +643,7 @@ aarch64_init_simd_builtin_types (void)
/* Poly types are a world of their own. */
aarch64_simd_types[Poly8_t].eltype = aarch64_simd_types[Poly8_t].itype =
  build_distinct_type_copy (unsigned_intQI_type_node);
 +  TYPE_STRING_FLAG (aarch64_simd_types[Poly8_t].eltype) = false;
aarch64_simd_types[Poly16_t].eltype =
 aarch64_simd_types[Poly16_t].itype =
  build_distinct_type_copy (unsigned_intHI_type_node);
aarch64_simd_types[Poly64_t].eltype =
 aarch64_simd_types[Poly64_t].itype =
>>
>> This fix seems correct to me, the poly types are not strings. Looking at
>> other uses of TYPE_STRING_FLAG this change doesn't seem like it would have
>> impact on parsing or code generation.
>>
>> OK for trunk.
>
> I committed this in r263561.
>

happens on arm too.

Same as r263561, but for arm: avoid compilation errors caused by poly
initializers getting treated as string literals.

Tested on arm-none-linux-gnueabihf.

gcc/ChangeLog:
2018-08-17  Szabolcs Nagy  

* config/arm/arm-builtins.c (arm_init_simd_builtin_types): Clear
polyNxK_t element's TYPE_STRING_FLAG.



diff --git a/gcc/config/arm/arm-builtins.c b/gcc/config/arm/arm-builtins.c
index 183a7b907f6..563ca51dcd0 100644
--- a/gcc/config/arm/arm-builtins.c
+++ b/gcc/config/arm/arm-builtins.c
@@ -927,6 +927,11 @@ arm_init_simd_builtin_types (void)
   (*lang_hooks.types.register_builtin_type) (arm_simd_polyTI_type_node,
 "__builtin_neon_poly128");
 
+  /* Prevent front-ends from transforming poly vectors into string

+ literals.  */
+  TYPE_STRING_FLAG (arm_simd_polyQI_type_node) = false;
+  TYPE_STRING_FLAG (arm_simd_polyHI_type_node) = false;
+
   /* Init all the element types built by the front-end.  */
   arm_simd_types[Int8x8_t].eltype = intQI_type_node;
   arm_simd_types[Int8x16_t].eltype = intQI_type_node;

Ok.
Thanks,
Kyrill

[PATCH] rs6000: Give names to cbranch/creturn patterns

[Segher Boessenkool]

This gives a name to the conditional branch and conditional return
patterns, so that it looks neater in dumps and verbose asm.  Also, the
comment for conditional branch was out of date; this fixes it.

Committing.


Segher


2018-08-17  Segher Boessenkool  

* config/rs6000/rs6000.md (*cbranch, *creturn): Name these patterns
(they were unnamed before).  Fix comments.

---
 gcc/config/rs6000/rs6000.md | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index 57ee6d8..3f145f7 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -12315,11 +12315,10 @@ (define_insn_and_split "*nesi3_ext"
  (const_string "12")
  (const_string "16")))])
 
-;; Define both directions of branch and return.  If we need a reload
-;; register, we'd rather use CR0 since it is much easier to copy a
-;; register CC value to there.
+;; Conditional branches.
+;; These either are a single bc insn, or a bc around a b.
 
-(define_insn ""
+(define_insn "*cbranch"
   [(set (pc)
(if_then_else (match_operator 1 "branch_comparison_operator"
  [(match_operand 2 "cc_reg_operand" "y")
@@ -12339,7 +12338,8 @@ (define_insn ""
  (const_int 4)
  (const_int 8)))])
 
-(define_insn ""
+;; Conditional return.
+(define_insn "*creturn"
   [(set (pc)
(if_then_else (match_operator 0 "branch_comparison_operator"
  [(match_operand 1 "cc_reg_operand" "y")
-- 
1.8.3.1

[PATCH] Fix poly types after PR tree-optimization/71625 strlen optimization

[Szabolcs Nagy]

On 15/08/18 16:51, Martin Sebor wrote:

On 08/15/2018 04:28 AM, James Greenhalgh wrote:

On Tue, Aug 14, 2018 at 09:34:08PM -0500, Martin Sebor wrote:

On 08/14/2018 09:24 AM, Martin Sebor wrote:

On 08/14/2018 09:08 AM, Martin Sebor wrote:
--- gcc/config/aarch64/aarch64-builtins.c    (revision 263537)
+++ gcc/config/aarch64/aarch64-builtins.c    (working copy)
@@ -643,6 +643,7 @@ aarch64_init_simd_builtin_types (void)
   /* Poly types are a world of their own.  */
   aarch64_simd_types[Poly8_t].eltype = aarch64_simd_types[Poly8_t].itype =
 build_distinct_type_copy (unsigned_intQI_type_node);
+  TYPE_STRING_FLAG (aarch64_simd_types[Poly8_t].eltype) = false;
   aarch64_simd_types[Poly16_t].eltype =
aarch64_simd_types[Poly16_t].itype =
 build_distinct_type_copy (unsigned_intHI_type_node);
   aarch64_simd_types[Poly64_t].eltype =
aarch64_simd_types[Poly64_t].itype =


This fix seems correct to me, the poly types are not strings. Looking at
other uses of TYPE_STRING_FLAG this change doesn't seem like it would have
impact on parsing or code generation.

OK for trunk.


I committed this in r263561.



happens on arm too.

Same as r263561, but for arm: avoid compilation errors caused by poly
initializers getting treated as string literals.

Tested on arm-none-linux-gnueabihf.

gcc/ChangeLog:
2018-08-17  Szabolcs Nagy  

* config/arm/arm-builtins.c (arm_init_simd_builtin_types): Clear
polyNxK_t element's TYPE_STRING_FLAG.
diff --git a/gcc/config/arm/arm-builtins.c b/gcc/config/arm/arm-builtins.c
index 183a7b907f6..563ca51dcd0 100644
--- a/gcc/config/arm/arm-builtins.c
+++ b/gcc/config/arm/arm-builtins.c
@@ -927,6 +927,11 @@ arm_init_simd_builtin_types (void)
   (*lang_hooks.types.register_builtin_type) (arm_simd_polyTI_type_node,
 	 "__builtin_neon_poly128");
 
+  /* Prevent front-ends from transforming poly vectors into string
+ literals.  */
+  TYPE_STRING_FLAG (arm_simd_polyQI_type_node) = false;
+  TYPE_STRING_FLAG (arm_simd_polyHI_type_node) = false;
+
   /* Init all the element types built by the front-end.  */
   arm_simd_types[Int8x8_t].eltype = intQI_type_node;
   arm_simd_types[Int8x16_t].eltype = intQI_type_node;

Re: [committed] diagnostics: add line numbers to source (PR other/84889)

[Andreas Schwab]

On Aug 17 2018, Manuel López-Ibáñez  wrote:

> However, I see that GCC trunk still counts tabs as 1-column, probably
> because emacs counts tabs as one column when interpreting column numbers
> in the output of GCC.

That is not true.  Emacs is using screen columns by default for almost
20 years now (see compilation-error-screen-columns).

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[PATCH,rs6000] Add builtins for accessing the FPSCR

[Carl Love]

GCC maintainers:

The following patch adds builtins to change the value of the FPSCR.
Specifically, __builtin_set_fpscr_rn, __builtin_set_fpscr_drn,
__builtin_mffsl, __builtin_mtfsb0,  __builtin_mtfsb1.  I added
documentation on the builtins in extend.texi.  In addition to listing
the builtin, I added a C style comment to describe the builtin a
little.  I don't see any of the other builtins documented like this. 
But I felt some explanation of the builtins were helpful.  Suggestions
on a better way to add the comments on the builtins would be
appreciated.

The patch has been tested on 

powerpc64le-unknown-linux-gnu (Power 8 LE) 
    powerpc64le-unknown-linux-gnu (Power 9 LE)

With no regressions.

Please let me know if the patch looks OK for trunk.

 Carl Love
-

gcc/ChangeLog:

2018-08-16  Carl Love  

* config/rs6000/rs6000-builtin.def: Add definitions for __builtin_mffsl,
__builtin_mtfsb0, __builtin_mtfsb1, __builtin_set_fpscr_rn,
__builtin_set_fpscr_drn.
* config/rs6000.c: Add functions rs6000_expand_mtfsb0_mtfsb1_builtin,
rs6000_expand_set_fpscr_rn_builtin, rs6000_expand_set_fpscr_drn_builtin.
Add case statement entries for the new builtins.
* config/rs6000.md: Add define_insn for rs6000_mtfsb0_si,
rs6000_mtfsb1_si, rs6000_mffscrn, rs6000_mffscdrn.
Add define_expand for rs6000_set_fpscr_rn and rs6000_set_fpscr_drn.
* doc/extend.texi: Add documentation for the builtins.

gcc/testsuite/ChangeLog:

2018-08-16  Carl Love  

* testsuite/gcc.target/powerpc/test_mffsl-p9.c: New file.
* testsuite/gcc.target/powerpc/test_fpscr_builtins.c: New file.
* testsuite/gcc.target/powerpc/test_fpscr_builtins_error.c: New file.
---
 gcc/config/rs6000/rs6000-builtin.def  |  23 ++
 gcc/config/rs6000/rs6000.c| 151 ++
 gcc/config/rs6000/rs6000.md   | 149 -
 gcc/doc/extend.texi   |  20 ++
 .../gcc.target/powerpc/test_fpscr_builtins.c  | 282 ++
 .../powerpc/test_fpscr_builtins_error.c   |  26 ++
 .../gcc.target/powerpc/test_mffsl-p9.c|  36 +++
 7 files changed, 686 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/test_fpscr_builtins.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/test_fpscr_builtins_error.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/test_mffsl-p9.c

diff --git a/gcc/config/rs6000/rs6000-builtin.def 
b/gcc/config/rs6000/rs6000-builtin.def
index f79968154..a50236e77 100644
--- a/gcc/config/rs6000/rs6000-builtin.def
+++ b/gcc/config/rs6000/rs6000-builtin.def
@@ -2486,11 +2486,34 @@ BU_SPECIAL_X (RS6000_BUILTIN_MFTB, "__builtin_ppc_mftb",
 BU_SPECIAL_X (RS6000_BUILTIN_MFFS, "__builtin_mffs",
  RS6000_BTM_ALWAYS, RS6000_BTC_MISC)
 
+BU_SPECIAL_X (RS6000_BUILTIN_MFFSL, "__builtin_mffsl",
+ RS6000_BTM_ALWAYS, RS6000_BTC_MISC)
+
 RS6000_BUILTIN_X (RS6000_BUILTIN_MTFSF, "__builtin_mtfsf",
  RS6000_BTM_ALWAYS,
  RS6000_BTC_MISC | RS6000_BTC_UNARY | RS6000_BTC_VOID,
  CODE_FOR_rs6000_mtfsf)
 
+RS6000_BUILTIN_X (RS6000_BUILTIN_MTFSB0_SI, "__builtin_mtfsb0",
+ RS6000_BTM_ALWAYS,
+ RS6000_BTC_MISC | RS6000_BTC_UNARY,
+ CODE_FOR_rs6000_mtfsb0_si)
+
+RS6000_BUILTIN_X (RS6000_BUILTIN_MTFSB1_SI, "__builtin_mtfsb1",
+ RS6000_BTM_ALWAYS,
+ RS6000_BTC_MISC | RS6000_BTC_UNARY,
+ CODE_FOR_rs6000_mtfsb1_si)
+
+RS6000_BUILTIN_X (RS6000_BUILTIN_SET_FPSCR_RN, "__builtin_set_fpscr_rn",
+ RS6000_BTM_ALWAYS,
+ RS6000_BTC_MISC | RS6000_BTC_UNARY,
+ CODE_FOR_rs6000_set_fpscr_rn)
+
+RS6000_BUILTIN_X (RS6000_BUILTIN_SET_FPSCR_DRN, "__builtin_set_fpscr_drn",
+ RS6000_BTM_ALWAYS,
+ RS6000_BTC_MISC | RS6000_BTC_UNARY,
+ CODE_FOR_rs6000_set_fpscr_drn)
+
 BU_SPECIAL_X (RS6000_BUILTIN_CPU_INIT, "__builtin_cpu_init",
  RS6000_BTM_ALWAYS, RS6000_BTC_MISC)
 
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index aa707b255..7db9c10a9 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -13356,6 +13356,113 @@ rs6000_expand_mtfsf_builtin (enum insn_code icode, 
tree exp)
   return NULL_RTX;
 }
 
+static rtx
+rs6000_expand_mtfsb0_mtfsb1_builtin (enum insn_code icode, tree exp)
+{
+  rtx pat;
+  tree arg0 = CALL_EXPR_ARG (exp, 0);
+  rtx op0 = expand_normal (arg0);
+  machine_mode mode0 = insn_data[icode].operand[0].mode;
+
+  if (icode == CODE_FOR_nothing)
+/* Builtin not supported on this processor.  */
+return 0;
+
+  /* If we got invalid arguments bail out before generating bad rtl.  */
+  if (arg0 == error_mark_node)
+return const0_rtx;
+
+  /* Only allow bit numbers 0 to 31.  */
+  

Re: [PATCH, OpenACC] Add support for gang local storage allocation in shared memory

[Bernhard Reutner-Fischer]

On 16 August 2018 17:46:43 CEST, Julian Brown  wrote:
>On Wed, 15 Aug 2018 21:56:54 +0200
>Bernhard Reutner-Fischer  wrote:
>
>> On 15 August 2018 18:46:37 CEST, Julian Brown
>>  wrote:
>> >On Mon, 13 Aug 2018 12:06:21 -0700
>> >Cesar Philippidis  wrote:  
>> 
>> atttribute has more t than strictly necessary. 
>> Don't like signed integer levels where they should be some unsigned. 
>> Also don't like single switch cases instead of if.
>> And omitting function comments even if the hook way above is
>> documented may be ok ish but is a bit lazy ;)
>
>Here's a new version with those comments addressed. I also changed the
>logic around a little to avoid adding decls to the vec in omp_context
>which would never be given the gang-private attribute.
>
>Re-tested with offloading to NVPTX.
>
>OK?

(TREE_CODE (var) == VAR_DECL
Is nowadays known as VAR_P (decl), FWIW.

ISTM that global variables are not JIT-friendly.
No further comments from me.

Thanks,

Re: [committed] diagnostics: add line numbers to source (PR other/84889)

[Manuel López-Ibáñez]

On 09/08/18 21:09, David Malcolm wrote:


It turns out that we convert tab characters to *single* space
characters when printing source code.

This behavior has been present since Manu first implemented
-fdiagnostics-show-caret in r186305 (aka
5a9830842f69ebb059061e26f8b0699cbd85121e, PR 24985), where it was this
logic (there in diagnostic.c's diagnostic_show_locus):
   char c = *line == '\t' ? ' ' : *line;
   pp_character (context->printer, c);

(that logic is now in diagnostic-show-locus.c in
layout::print_source_line)

Arguably this is a bug, but it's intimately linked to the way in which
we track "column numbers".  Our "column numbers" are currently simply a
1-based byte-count, I believe, so a tab character is treated by us as
simply an increment of 1 right now.  There are similar issues with
multibyte characters, which are being tracked in PR 49973.



Hi David,

At the time, this was done on purpose for two reasons:

1) The way we counted column numbers already counted tabs as 1-space and ...
2) It leads to wasting less horizontal space and more consistent output.

I believe that (1) was due to this bug: 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52899 which got fixed.


The GCS says that column numbers should count tab stops as 8 spaces 
[https://www.gnu.org/prep/standards/html_node/Errors.html].

I believe that -ftabstop=8 is the default after PR52899 was fixed.

However, I see that GCC trunk still counts tabs as 1-column, probably because 
emacs counts tabs as one column when interpreting column numbers in the output 
of GCC. As long as both of these things are true, I believe it doesn't make 
much sense to print 8 spaces (or a tab) instead of a 1-column space. It will 
make interpreting the column numbers much harder and break the parsing of GCC 
diagnostics done by emacs.


Note that if we print the tab directly, the width of the tab in the terminal 
may not be the same as in the editor the user is using. Moreover, if the user 
is using tabs consistently (instead of using tabs on some lines and spaces in 
others), replacing tabs with 1 space will only reduce the visual space per 
indentation level, but the indentation structure will remain consistent.


I wish I had added a summary of the above to the code as a comment.

Finally, PR49973 is about GCC counting multiple columns for characters that 
should be counted as one column. This should be fixed in our line-map 
implementation using wcwidth() when lexing. It is not the same issue at all. 
Once column numbers are correctly counted, the output should be fine as well 
(the caret line does not change multi-byte characters).


I hope the above helps,

Manuel.

[PATCH] Macro body is trailing array

[Nathan Sidwell]

This patch changes the representation of a macro body, rather than be a 
pointer to an array of tokens, it is a trailing array of tokens.  That's 
only for iso macros, traditional ones are still a pointer to a 
[segmented] char array.


I introduce two helpers to deal with allocation (cpp uses an 
obstack-like mechanism).  We now have to allocate the macro object after 
parsing the parms, so the macro definition functions get reordered a bit.


the new two-value enumeration 'macro_kind' and its storage in a 2-bit 
bitfield is a bit of foreshadowing :)


A couple of uses needed constifying, as we now inherit the qualification 
of the containing cpp_macro object when looking at its tokens.


booted & tested on x86_64-linux.

nathan

--
Nathan Sidwell
2018-08-17  Nathan Sidwell  

	libcpp/
	* include/cpplib.h (enum cpp_macro_kind): New.
	(struct cpp_macro): Make body trailing array.  Add kind field,
	delete traditional flag.
	* internal.h (_cpp_new_macro): Declare.
	(_cpp_reserve_room): New inline.
	(_cpp_commit_buf): Declare.
	(_cpp_create_trad_definition): Return new macro.
	* lex.c (_cpp_commit_buff): New.
	* macro.c (macro_real_token_count): Count backwards.
	(replace_args): Pointer equality not orderedness.
	(_cpp_save_parameter): Use _cpp_reserve_room.
	(alloc_expansion_token): Delete.
	(lex_expansion_token): Return macro pointer.  Use _cpp_reserve_room.
	(create_iso_definition): Allocate macro itself.  Adjust for
	different allocation ordering.
	(_cpp_new_macro): New.
	(_cpp_create_definition): Adjust for API changes.
	* traditional.c (push_replacement_text): Don't set traditional
	flag.
	(save_replacement_text): Likewise.
	(_cpp_create_trad_definition): Allocate macro itself, Adjust for
	different allocation ordering.
	gcc/c-family/
	* c-ada-spec.c (macro_length, dump_ada_macros): Constify.

Index: gcc/c-family/c-ada-spec.c
===
--- gcc/c-family/c-ada-spec.c	(revision 263618)
+++ gcc/c-family/c-ada-spec.c	(working copy)
@@ -88,7 +88,7 @@ macro_length (const cpp_macro *macro, in
 
   for (j = 0; j < macro->count; j++)
 {
-  cpp_token *token = >exp.tokens[j];
+  const cpp_token *token = >exp.tokens[j];
 
   if (token->flags & PREV_WHITE)
 	(*buffer_len)++;
@@ -274,7 +274,7 @@ dump_ada_macros (pretty_printer *pp, con
 
 	  for (i = 0; supported && i < macro->count; i++)
 	{
-	  cpp_token *token = >exp.tokens[i];
+	  const cpp_token *token = >exp.tokens[i];
 	  int is_one = 0;
 
 	  if (token->flags & PREV_WHITE)
Index: libcpp/include/cpplib.h
===
--- libcpp/include/cpplib.h	(revision 263618)
+++ libcpp/include/cpplib.h	(working copy)
@@ -671,6 +671,12 @@ struct cpp_dir
   dev_t dev;
 };
 
+/* The kind of the cpp_macro.  */
+enum cpp_macro_kind {
+  cmk_macro,	/* An ISO macro (token expansion).  */
+  cmk_traditional,	/* A traditional macro (text expansion).  */
+};
+
 /* Each macro definition is recorded in a cpp_macro structure.
Variadic macros cannot occur with traditional cpp.  */
 struct GTY(()) cpp_macro {
@@ -683,15 +689,6 @@ struct GTY(()) cpp_macro {
 			length ("%h.paramc")))
 params;
 
-  /* Replacement tokens (ISO) or replacement text (traditional).  See
- comment at top of cpptrad.c for how traditional function-like
- macros are encoded.  */
-  union cpp_macro_u
-  {
-cpp_token * GTY ((tag ("0"), length ("%0.count"))) tokens;
-const unsigned char * GTY ((tag ("1"))) text;
-  } GTY ((desc ("%1.traditional"))) exp;
-
   /* Definition line number.  */
   source_location line;
 
@@ -701,6 +698,9 @@ struct GTY(()) cpp_macro {
   /* Number of parameters.  */
   unsigned short paramc;
 
+  /* The kind of this macro (ISO, trad or assert) */
+  unsigned kind : 2;
+
   /* If a function-like macro.  */
   unsigned int fun_like : 1;
 
@@ -713,13 +713,23 @@ struct GTY(()) cpp_macro {
   /* Nonzero if it has been expanded or had its existence tested.  */
   unsigned int used : 1;
 
-  /* Indicate which field of 'exp' is in use.  */
-  unsigned int traditional : 1;
-
   /* Indicate whether the tokens include extra CPP_PASTE tokens at the
  end to track invalid redefinitions with consecutive CPP_PASTE
  tokens.  */
   unsigned int extra_tokens : 1;
+
+  /* 1 bits spare (32-bit). 33 on 64-bit target.  */
+
+  union cpp_exp_u
+  {
+/* Trailing array of replacement tokens (ISO), or assertion body value.  */
+cpp_token GTY ((tag ("false"), length ("%1.count"))) tokens[1];
+
+/* Pointer to replacement text (traditional).  See comment at top
+   of cpptrad.c for how traditional function-like macros are
+   encoded.  */
+const unsigned char *GTY ((tag ("true"))) text;
+  } GTY ((desc ("%1.kind == cmk_traditional"))) exp;
 };
 
 /* The structure of a node in the hash table.  The hash table has
Index: libcpp/internal.h
===
--- 

Re: [PATCH] Make strlen range computations more conservative

[Martin Sebor]

On 08/17/2018 04:31 AM, Richard Biener wrote:

On Tue, 7 Aug 2018, Martin Sebor wrote:


On 08/07/2018 11:44 AM, Richard Biener wrote:

On August 7, 2018 4:37:00 PM GMT+02:00, Martin Sebor 
wrote:

On 08/07/2018 02:51 AM, Richard Biener wrote:

On August 7, 2018 4:24:42 AM GMT+02:00, Martin Sebor

 wrote:

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

On Wed, 25 Jul 2018, Martin Sebor wrote:


BUT - for the string_constant and c_strlen functions we are,
in all cases we return something interesting, able to look
at an initializer which then determines that type.  Hopefully.
I think the strlen() folding code when it sets SSA ranges
now looks at types ...?

Consider

struct X { int i; char c[4]; int j;};
struct Y { char c[16]; };

void foo (struct X *p, struct Y *q)
{
  memcpy (p, q, sizeof (struct Y));
  if (strlen ((char *)(struct Y *)p + 4) < 7)
abort ();
}

here the GIMPLE IL looks like

  const char * _1;

   [local count: 1073741825]:
  _5 = MEM[(char * {ref-all})q_4(D)];
  MEM[(char * {ref-all})p_6(D)] = _5;
  _1 = p_6(D) + 4;
  _2 = __builtin_strlen (_1);

and I guess Martin would argue that since p is of type struct X
+ 4 gets you to c[4] and thus strlen of that cannot be larger
than 3.  But of course the middle-end doesn't work like that
and luckily we do not try to draw such conclusions or we
are somehow lucky that for the testcase as written above we do

not

(I'm not sure whether Martins changes in this area would derive
such conclusions in principle).


Only if the strlen argument were p->c.


NOTE - we do not know the dynamic type here since we do not know
the dynamic type of the memory pointed-to by q!  We can only
derive that at q+4 there must be some object that we can
validly call strlen on (where Martin again thinks strlen
imposes constrains that memchr does not - sth I do not agree
with from a QOI perspective)


The dynamic type is a murky area.


It's well-specified in the middle-end.  A store changes the
dynamic type of the stored-to object.  If that type is
compatible with the surrounding objects dynamic type that one
is not affected, if not then the surrounding objects dynamic
type becomes unspecified.  There is TYPE_TYPELESS_STORAGE
to somewhat control "compatibility" of subobjects.


I never responded to this.  Using a dynamic (effective) type as
you describe it would invalidate the aggressive loop optimization
in the following:

  void foo (struct X *p)
  {
  struct Y y = { "12345678" };
  memcpy (p, , sizeof (struct Y));

  // *p effective type is now struct Y

  int n = 0;
  while (p->c[n])
++n;

  if (n < 7)
abort ();
  }

GCC unconditionally aborts, just as it does with strlen(p->c).
Why is that not wrong (in either case)?

Because the code is invalid either way, for two reasons:


No, because the storage has only 4 non-null characters starting at

offset 4?

No, for the reasons below.  I made a mistake of making
the initializer string too short.  If we make it longer it
still aborts.  Say with this

  struct Y y = { "123456789012345" };

we end up with this DCE:

 struct Y y;

   :
  MEM[(char * {ref-all})p_6(D)] = 0x353433323130393837363534333231;
  __builtin_abort ();

With -fdump-tree-cddce1-details (and a patch to show the upper
bound) we see:

  Found loop 1 to be finite: upper bound found: 3.

With -fno-aggressive-loop-optimizations the abort becomes
conditional because the array bound isn't considered. I would
expect you to know this since you implemented the feature.


Honza added the array indexing part and it may very well be too aggressive.
I have to take a closer look after vacation to tell. Can you open a PR and
CC me there?


I opened bug 86884.


Now that I returned from vacation the testcase is simply bogus.  The
access p->c[n] requires an affective type of X.


I agree.  And so does strlen(p->c).  It was your example
(with strlen) to illustrate a valid use case.

Saying that the loop is invalid when p->c doesn't contain
zero:

  int n = 0;
  while (p->c[n])
++n;

means that the equivalent loop below is also invalid:

  int n = 0;
  for (const char *q = p->c; *q; ++q)
++n;

The three (the two loops and the strlen(p->c) call) are
all equivalent and semantically identical.

Since one of them is invalid, the other two must be as well.
It's not wrong to emit different code for equivalent invalid
constructs, but as a matter of a QoI, it makes no sense to
_guarantee_ different semantics for constructs that, at
the C/C++ language level, are semantically equivalent, just
because they use subtly different syntax.

Put another way, a programmer who uses strlen(p->c) in this
context and who is assured that it's valid code should be
free to replace that call with a loop and have it continue
to work without change.

Martin

Re: Async I/O patch with compilation fix

[Thomas Koenig]

Hi Christophe,

sorry that this took so long, but a holiday followed by a
business trip seven timezones away can do that :-)


I applied this patch, and again I still see regressions on
armeb-none-linux-gnueabihf
--with-cpu cortex-a9
--with-fpu neon-fp16


The info that you supplied in the PR indicates some sort of library
problem exposed by the patch, possibly by including gthr.h.

All Nicolas and I could come up with was to remove the async I/O
functionality from armeb-* and by xfailing the tests.

This is done by

+#if defined(__GTHREAD_HAS_COND) && defined(__GTHREADS_CXX0X) && 
!defined(__ARMEB__)

+#define ASYNC_IO 1
+#else
+#define ASYNC_IO 0
+#endif

If somebody comes up with something more fine-grained for the
feature test, we can put this in now or later.

Regression-tested on x86_64-pc-linux-gnu (which showed that
xfail lines in the testsuite aren't wildly inaccurate).

So, I'd appreciate testing. If this passes, this will be
committed ASAP.

Regards

Thomas

Index: gcc/fortran/gfortran.texi
===
--- gcc/fortran/gfortran.texi	(Revision 263618)
+++ gcc/fortran/gfortran.texi	(Arbeitskopie)
@@ -879,8 +879,7 @@ than @code{(/.../)}.  Type-specification for array
 @item Extensions to the specification and initialization expressions,
 including the support for intrinsics with real and complex arguments.
 
-@item Support for the asynchronous input/output syntax; however, the
-data transfer is currently always synchronously performed. 
+@item Support for the asynchronous input/output.
 
 @item
 @cindex @code{FLUSH} statement
@@ -1183,6 +1182,7 @@ might in some way or another become visible to the
 * Files opened without an explicit ACTION= specifier::
 * File operations on symbolic links::
 * File format of unformatted sequential files::
+* Asynchronous I/O::
 @end menu
 
 
@@ -1486,6 +1486,20 @@ program main
 end program main
 @end smallexample
 
+@node Asynchronous I/O
+@section Asynchronous I/O
+@cindex input/output, asynchronous
+@cindex asynchronous I/O
+
+Asynchronous I/O is supported if the program is linked against the
+POSIX thread library. If that is not the case, all I/O is performed
+as synchronous.
+
+On some systems, such as Darwin or Solaris, the POSIX thread library
+is always linked in, so asynchronous I/O is always performed. On other
+sytems, such as Linux, it is necessary to specify @option{-pthread},
+@option{-lpthread} or @option{-fopenmp} during the linking step.
+
 @c -
 @c Extensions
 @c -
Index: gcc/fortran/trans-decl.c
===
--- gcc/fortran/trans-decl.c	(Revision 263618)
+++ gcc/fortran/trans-decl.c	(Arbeitskopie)
@@ -698,7 +698,8 @@ gfc_finish_var_decl (tree decl, gfc_symbol * sym)
 	  && CLASS_DATA (sym)->ts.u.derived->attr.has_dtio_procs)))
 TREE_STATIC (decl) = 1;
 
-  if (sym->attr.volatile_)
+  /* Treat asynchronous variables the same as volatile, for now.  */
+  if (sym->attr.volatile_ || sym->attr.asynchronous)
 {
   TREE_THIS_VOLATILE (decl) = 1;
   TREE_SIDE_EFFECTS (decl) = 1;
Index: gcc/fortran/trans-io.c
===
--- gcc/fortran/trans-io.c	(Revision 263618)
+++ gcc/fortran/trans-io.c	(Arbeitskopie)
@@ -438,10 +438,9 @@ gfc_build_io_library_fndecls (void)
 	get_identifier (PREFIX("st_iolength")), ".w",
 	void_type_node, 1, dt_parm_type);
 
-  /* TODO: Change when asynchronous I/O is implemented.  */
   parm_type = build_pointer_type (st_parameter[IOPARM_ptype_wait].type);
   iocall[IOCALL_WAIT] = gfc_build_library_function_decl_with_spec (
-	get_identifier (PREFIX("st_wait")), ".X",
+	get_identifier (PREFIX("st_wait_async")), ".w",
 	void_type_node, 1, parm_type);
 
   parm_type = build_pointer_type (st_parameter[IOPARM_ptype_filepos].type);
@@ -1527,7 +1526,7 @@ gfc_trans_wait (gfc_code * code)
 mask |= IOPARM_common_err;
 
   if (p->id)
-mask |= set_parameter_value (, var, IOPARM_wait_id, p->id);
+mask |= set_parameter_ref (, _block, var, IOPARM_wait_id, p->id);
 
   set_parameter_const (, var, IOPARM_common_flags, mask);
 
Index: gcc/testsuite/gfortran.dg/f2003_inquire_1.f03
===
--- gcc/testsuite/gfortran.dg/f2003_inquire_1.f03	(Revision 263618)
+++ gcc/testsuite/gfortran.dg/f2003_inquire_1.f03	(Arbeitskopie)
@@ -7,10 +7,12 @@ logical :: vpending
 open(10, file='mydata_f2003_inquire_1', asynchronous="yes", blank="null", &
 & decimal="comma", encoding="utf-8", sign="plus")
 
+write (10,*, asynchronous="yes", id=vid) 'asdf'
+wait (10)
+
 inquire(unit=10, round=sround, sign=ssign, size=vsize, id=vid, &
 & pending=vpending, asynchronous=sasynchronous, decimal=sdecimal, &
 & encoding=sencoding)
-
 if (ssign.ne."PLUS") STOP 1
 if 

Re: [PATCH, rs6000] Early gimple folding of vec_mergeh and vec_mergel for float

[Richard Biener]

On August 17, 2018 5:02:53 PM GMT+02:00, Will Schmidt 
 wrote:
>On Fri, 2018-08-17 at 16:00 +0200, Richard Biener wrote:
>> On Wed, Aug 8, 2018 at 12:59 AM Segher Boessenkool
>>  wrote:
>> >
>> > Hi!
>> >
>> > On Tue, Aug 07, 2018 at 02:24:58PM -0500, Will Schmidt wrote:
>> > >This adds support for gimple folding of vec_mergeh and
>vec_mergel
>> > > for float and double types.   Support for the integral types is
>already
>> > > in-tree.
>> >
>> > > +  /* The permute_type will match the lhs for integral types. 
>For double and
>> > > + float types, the permute type needs to map to the V2 or V4
>type that
>> > > + matches size.  */
>> > > +  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))
>> > > +  permute_type = V2DI_type_node;
>> > > +else if (TREE_TYPE (lhs_type) == TREE_TYPE (V4SF_type_node))
>> > > +  permute_type = V4SI_type_node;
>> > > +else
>> > > +  gcc_unreachable ();
>> >
>> > Please write this as
>> >
>> >   if (INTEGRAL_TYPE_P (TREE_TYPE (lhs_type)))
>> > permute_type = lhs_type;
>> >   else if (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))
>> > permute_type = V4SI_type_node;
>> >   else
>> > gcc_unreachable ();
>> >
>> > or, if you want to emphasize integer vs. float:
>> >
>> >   if (INTEGRAL_TYPE_P (TREE_TYPE (lhs_type)))
>> > permute_type = lhs_type;
>> >   else
>> > {
>> >   if (TREE_TYPE (lhs_type) == TREE_TYPE (V2DF_type_node))
>> 
>> Are you sure lhs_type is never qualified?
>
>For the V2DF and V4SF types, I am mostly sure, but since you bring it
>up
>I will admit I am not positive.  :-)   
>
>> That is, for a GIMPLE folder
>> I'd have expected
>>
>>   if (types_compatible_p (TREE_TYPE (lhs_type), TREE_TYPE
>(V2DF_type_node)))
>
>> for GENERIC
>> 
>>   if (TYPE_MAIN_VARIANT (TREE_TYPE (lhs_type)) == TREE_TYPE
>(V2DF_type_node))
>
>Either/both of those seem more robust than what I had come up with,
>I'll
>plan to make an update here.
>
>Any guidance on whether I should prefer "types_compatible_p" versus the
>GENERIC "TYPE_MAIN_VARIANT" ?   I see more references to the latter in
>rs6000.c , but the former seems to make better sense to me just by its
>name.

The former if this is called from the GIMPLE stmts folding hook. 

Richard. 

>Thanks for the review, 
>-Will
>
>
>> Richard.
>> 
>> > permute_type = V2DI_type_node;
>> >   else if (TREE_TYPE (lhs_type) == TREE_TYPE (V4SF_type_node))
>> > permute_type = V4SI_type_node;
>> >   else
>> > gcc_unreachable ();
>> > }
>> >
>> > Okay for trunk with that changed.  Thanks!
>> >
>> >
>> > Segher
>> 

Re: [PATCH, rs6000] Early gimple folding of vec_mergeh and vec_mergel for float

[Will Schmidt]

On Fri, 2018-08-17 at 16:00 +0200, Richard Biener wrote:
> On Wed, Aug 8, 2018 at 12:59 AM Segher Boessenkool
>  wrote:
> >
> > Hi!
> >
> > On Tue, Aug 07, 2018 at 02:24:58PM -0500, Will Schmidt wrote:
> > >This adds support for gimple folding of vec_mergeh and vec_mergel
> > > for float and double types.   Support for the integral types is already
> > > in-tree.
> >
> > > +  /* The permute_type will match the lhs for integral types.  For double 
> > > and
> > > + float types, the permute type needs to map to the V2 or V4 type that
> > > + matches size.  */
> > > +  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))
> > > +  permute_type = V2DI_type_node;
> > > +else if (TREE_TYPE (lhs_type) == TREE_TYPE (V4SF_type_node))
> > > +  permute_type = V4SI_type_node;
> > > +else
> > > +  gcc_unreachable ();
> >
> > Please write this as
> >
> >   if (INTEGRAL_TYPE_P (TREE_TYPE (lhs_type)))
> > permute_type = lhs_type;
> >   else if (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))
> > permute_type = V4SI_type_node;
> >   else
> > gcc_unreachable ();
> >
> > or, if you want to emphasize integer vs. float:
> >
> >   if (INTEGRAL_TYPE_P (TREE_TYPE (lhs_type)))
> > permute_type = lhs_type;
> >   else
> > {
> >   if (TREE_TYPE (lhs_type) == TREE_TYPE (V2DF_type_node))
> 
> Are you sure lhs_type is never qualified?

For the V2DF and V4SF types, I am mostly sure, but since you bring it up
I will admit I am not positive.  :-)   

> That is, for a GIMPLE folder
> I'd have expected
>
>   if (types_compatible_p (TREE_TYPE (lhs_type), TREE_TYPE (V2DF_type_node)))

> for GENERIC
> 
>   if (TYPE_MAIN_VARIANT (TREE_TYPE (lhs_type)) == TREE_TYPE (V2DF_type_node))

Either/both of those seem more robust than what I had come up with, I'll
plan to make an update here.

Any guidance on whether I should prefer "types_compatible_p" versus the
GENERIC "TYPE_MAIN_VARIANT" ?   I see more references to the latter in
rs6000.c , but the former seems to make better sense to me just by its
name.

Thanks for the review, 
-Will


> Richard.
> 
> > permute_type = V2DI_type_node;
> >   else if (TREE_TYPE (lhs_type) == TREE_TYPE (V4SF_type_node))
> > permute_type = V4SI_type_node;
> >   else
> > gcc_unreachable ();
> > }
> >
> > Okay for trunk with that changed.  Thanks!
> >
> >
> > Segher
> 

Re: PING [PATCH] warn for strlen of arrays with missing nul (PR 86552, 86711, 86714) )

[Martin Sebor]

On 08/16/2018 11:14 PM, Jeff Law wrote:

On 08/01/2018 08:44 PM, Martin Sebor wrote:

Since the foundation of the patch is detecting and avoiding
the overly aggressive folding of unterminated char arrays,
besides issuing a warning for such arguments to strlen,
the patch also fixes pr86711 - wrong folding of memchr, and
pr86714 - tree-ssa-forwprop.c confused by too long initializer.

The substance of the attached updated patch is unchanged,
I have just added test cases for the two additional bugs.


Just to be absolutely sure.  The patch attached to this message is
superceded by the later 6 part patchkit that fixes 86714, 86711 and 86552.

I'm just trying to make sure I'm looking at the right kits from both you
and Bernd for the next step.


Yes, I broke up this patch into a series, with the basic
"infrastructure" in part 1:

[PATCH 1/6] prevent folding of unterminated const arrays (PR
86711, 86714)
https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00769.html

and the strlen missing nul detection in part 2:
[PATCH 2/6] detect unterminated const arrays in strlen calls
(PR 86552)
https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00778.html

The rest of the series (parts 3 through 6) add the missing
nul detection to a few other built-ins.

Martin

Re: [PATCH] scev: dump final value replacement expressions

[Jeff Law]

On 08/17/2018 07:19 AM, Alexander Monakov wrote:
> Hello,
> 
> I'd like to apply the following patch to make GENERIC expressions
> used in final value replacement visible in pass dumps.
> 
> We currently dump only the last statement, but that is not very
> convenient as final values are sometimes complex and expand to
> multiple gimple statements.
> 
>   * tree-scalar-evolution.c (final_value_replacement_loop): Dump
>   full GENERIC expression used for replacement.
OK.
jeff

Re: [PATCH, RFC, rs6000] enable GIMPLE folding of vec_splat

[Richard Biener]

On Tue, Aug 7, 2018 at 9:25 PM Will Schmidt  wrote:
>
> Hi
> Enable GIMPLE folding of the vec_splat() intrinsic.
>
> For review.. feedback is expected. :-)
>
> I came up with the following after spending some time poking around
> at the tree_vec_extract() and vector_element() functions as seen
> in tree-vect-generic.c looking for insights.  Some of this seems a
> bit clunky to me yet, but this is functional as far as make-check
> can tell, and is as far as I can get without additional input.
>
> This uses the tree_vec_extract() function out of tree-vect-generic.c
> to retrieve the splat value, which is a BIT_FIELD_REF.   That function is
> made non-static as part of this change.
>
> In review of the .gimple output, this folding takes a sample testcase
> of
> vector bool int testb_0  (vector bool int x)
> {
>   return vec_splat (x, 0b0);
> }
> from:
> testb_0 (__vector __bool int x)
> {
>   __vector __bool intD.1486 D.2855;
>
>   _1 = VIEW_CONVERT_EXPR<__vector signed intD.1468>(xD.2778);
>   _2 = __builtin_altivec_vspltwD.1919 (_1, 0);
>   D.2855 = VIEW_CONVERT_EXPR<__vector __bool intD.1486>(_2);
>   return D.2855;
> }
> to:
>  testb_0 (__vector __bool int x)
> {
>   __vector __bool intD.1486 D.2855;
>
>   _1 = VIEW_CONVERT_EXPR<__vector signed intD.1468>(xD.2778);
>   D.2856 = BIT_FIELD_REF <_1, 32, 0>;
>   _2 = {D.2856, D.2856, D.2856, D.2856};
>   D.2855 = VIEW_CONVERT_EXPR<__vector __bool intD.1486>(_2);
>   return D.2855;
> }
>
>
> Testcases are being posted as a separate patch.
>
> OK for trunk?

It certainly works, nowadays we have VEC_DUPLICATE_EXPR though
so you could simply do

 _1 = VIEW_CONVERT_EXPR<__vector signed intD.1468>(xD.2778);
 D.2856 = BIT_FIELD_REF <_1, 32, 0>;
 D.2855 = VEC_DUPLICATE_EXPR <__vector __bool intD.1486> (D.2856);

not sure what variant is better for followup optimizations and whether
we already fold that { D.2856, D.2856, D.2856, D.2856 }; to VEC_DUPLICATE_EXPR.

Richard may know more.

Richard.

> Thanks,
> -Will
>
> [gcc]
>
> 2018-08-07  Will Schmidt  
>
> * config/rs6000/rs6000.c (rs6000_gimple_fold_builtin): Add support for
>   early gimple folding of vec_splat().
> * tree-vect-generic.c: Remove static from tree_vec_extract() 
> definition.
> * gimple-fold.h:  Add an extern define for tree_vec_extract().
>
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index 35c32be..acc6b49 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -15764,10 +15764,56 @@ rs6000_gimple_fold_builtin (gimple_stmt_iterator 
> *gsi)
>  tree splat_tree = build_vector_from_val (TREE_TYPE (lhs), 
> splat_value);
>  g = gimple_build_assign (lhs, splat_tree);
>  gimple_set_location (g, gimple_location (stmt));
>  gsi_replace (gsi, g, true);
>  return true;
> +   }
> +
> +/* Flavors of vec_splat.  */
> +// a = vec_splat (b, 0x3) ;  // a = { b[3],b[3],b[3],...};
> +case ALTIVEC_BUILTIN_VSPLTB:
> +case ALTIVEC_BUILTIN_VSPLTH:
> +case ALTIVEC_BUILTIN_VSPLTW:
> +case VSX_BUILTIN_XXSPLTD_V2DI:
> +case VSX_BUILTIN_XXSPLTD_V2DF:
> +  {
> +   arg0 = gimple_call_arg (stmt, 0); /* input vector.  */
> +   arg1 = gimple_call_arg (stmt, 1); /* index into arg0.  */
> +   /* Only fold the vec_splat_*() if arg1 is a constant
> +  5-bit unsigned literal.  */
> +   if (TREE_CODE (arg1) != INTEGER_CST || TREE_INT_CST_LOW (arg1) > 0x1f)
> + return false;
> +
> +   lhs = gimple_call_lhs (stmt);
> +   tree lhs_type = TREE_TYPE (lhs);
> +
> +   tree splat;
> +   if (TREE_CODE (arg0) == VECTOR_CST)
> + splat = VECTOR_CST_ELT (arg0, TREE_INT_CST_LOW (arg1));
> +   else
> + {
> +   /* Determine (in bits) the length and start location of the
> +  splat value for a call to the tree_vec_extract helper.  */
> +   int tree_size_in_bits = TREE_INT_CST_LOW (size_in_bytes 
> (lhs_type))
> +   * BITS_PER_UNIT;
> +   int splat_elem_size = tree_size_in_bits / VECTOR_CST_NELTS (arg0);
> +   int splat_start_bit = TREE_INT_CST_LOW (arg1) * splat_elem_size;
> +   /* Do not attempt to early-fold if the size + specified offset 
> into
> +  the vector would touch outside of the source vector.  */
> +   if ((splat_start_bit + splat_elem_size) > tree_size_in_bits)
> + return false;
> +   tree len = build_int_cst (bitsizetype, splat_elem_size);
> +   tree start = build_int_cst (bitsizetype, splat_start_bit);
> +   splat = tree_vec_extract (gsi, TREE_TYPE (lhs_type), arg0,
> + len, start);
> +   }
> +   /* And finally, build the new vector.  */
> +   tree splat_tree = build_vector_from_val (lhs_type, splat);
> +   g = gimple_build_assign (lhs, splat_tree);
> +   gimple_set_location (g, 

Re: [PATCH, rs6000] Early gimple folding of vec_mergeh and vec_mergel for float

[Richard Biener]

On Wed, Aug 8, 2018 at 12:59 AM Segher Boessenkool
 wrote:
>
> Hi!
>
> On Tue, Aug 07, 2018 at 02:24:58PM -0500, Will Schmidt wrote:
> >This adds support for gimple folding of vec_mergeh and vec_mergel
> > for float and double types.   Support for the integral types is already
> > in-tree.
>
> > +  /* The permute_type will match the lhs for integral types.  For double 
> > and
> > + float types, the permute type needs to map to the V2 or V4 type that
> > + matches size.  */
> > +  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))
> > +  permute_type = V2DI_type_node;
> > +else if (TREE_TYPE (lhs_type) == TREE_TYPE (V4SF_type_node))
> > +  permute_type = V4SI_type_node;
> > +else
> > +  gcc_unreachable ();
>
> Please write this as
>
>   if (INTEGRAL_TYPE_P (TREE_TYPE (lhs_type)))
> permute_type = lhs_type;
>   else if (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))
> permute_type = V4SI_type_node;
>   else
> gcc_unreachable ();
>
> or, if you want to emphasize integer vs. float:
>
>   if (INTEGRAL_TYPE_P (TREE_TYPE (lhs_type)))
> permute_type = lhs_type;
>   else
> {
>   if (TREE_TYPE (lhs_type) == TREE_TYPE (V2DF_type_node))

Are you sure lhs_type is never qualified?  That is, for a GIMPLE folder
I'd have expected

  if (types_compatible_p (TREE_TYPE (lhs_type), TREE_TYPE (V2DF_type_node)))

for GENERIC

  if (TYPE_MAIN_VARIANT (TREE_TYPE (lhs_type)) == TREE_TYPE (V2DF_type_node))

Richard.

> permute_type = V2DI_type_node;
>   else if (TREE_TYPE (lhs_type) == TREE_TYPE (V4SF_type_node))
> permute_type = V4SI_type_node;
>   else
> gcc_unreachable ();
> }
>
> Okay for trunk with that changed.  Thanks!
>
>
> Segher

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

[Bernd Edlinger]

On 08/17/18 15:38, Richard Biener wrote:
> On Fri, 17 Aug 2018, Bernd Edlinger wrote:
> 
>> On 08/17/18 14:19, Richard Biener wrote:
>>> On Fri, 17 Aug 2018, Bernd Edlinger wrote:
>>>
 Richard Biener wrote:
> +embedded @code{NUL} characters.  However, the
> +@code{TREE_STRING_LENGTH} always includes a trailing @code{NUL} that
> +is not part of the language string literal but appended by the front end.
> +If the string shall not be @code{NUL}-terminated the @code{TREE_TYPE}
> +is one character shorter than @code{TREE_STRING_LENGTH}.
> +Excess caracters other than one trailing @code{NUL} character are not
>>>
>>> characters btw.
>>>
>>
>> thanks, updated.
>>
>>> I read the above that the string literal for
>>>
>>> char x[2] = "1";
>>>
>>> is actually "1\0\0" - there's one NUL that is not part of the language
>>> string literal.  The second sentence then suggests that both \0
>>> are removed because 2 is less than 3?
>>>
>>
>> maybe 2 is a bad example, lets consider:
>> char x[20] = "1";
>>
>> That is a string_cst with STRING_LENGTH = 2, content = "2\0\0"
>> the array_type is used on both x, and the string_cst,
>> I was assuming that both tree objects refer to the same type object.
>> which is char[0..20-1]
> 
> Oh, didn't realize we use char[2] for the STRING_CST.  Makes
> my suggestion to use char[] instead not (very) much worse than the
> existing practice then.
> 
>> varasm assembles the bytes that are given by STRING_LENGTH
>> and appends zeros as appropriate.
>>
>>> As said, having this extra semantics of a STRING_CST tied to
>>> another tree node (its TREE_TYPE) looks ugly.
>>>
> +permitted.
>
> I find this very confusing and oppose to that change.  Can we get
> back to the drawing board please?  If we want an easy way to
> see whether a string is "properly" terminated then maybe we can
> simply use a flag that gets set by build_string?
>

 What I mean with that is the case like
 char x[2] = "123456";

 which is build_string(7, "123456"), but with a type char[2],
 so varasm throws away "3456\0".
>>>
>>> I think varasm throws away chars not because of the type of
>>> the STRING_CST but because of the available storage in x.
>>>
>>
>> But at other places we look at the type of the string_cst, don't we?
>> Shouldn't those be the same?
> 
> I think most (all?) places look at TREE_TYPE (TREE_TYPE (string))
> only.  I'm not aware of users of the array domain of the array type
> of a string - but I'm far from knowing GCC inside-out ;)
> 

Yes, I know, that happens to me as well on the first day after my holidays ;)

 I want to say that this is not okay, the excess precision
 should only be used to strip the nul termination, in cases
 where it is intended to be a assembled as a not zero terminated
 string.  But maybe the wording could be improved?
>>>
>>> ISTR we always assemble a NUL in .strings to get string merging
>>> working.
>>>
>>
>> String merging is not working when the string is not explicitly
>> NUL terminated, my followup patch here tries to fix that:
>>
>> [PATCH] Handle not explicitly zero terminated strings in merge sections
>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00481.html
> 
> I'd have expected sth as simple as
> 
>if (merge_strings && str[thissize - 1] != '\0')
>  thissize++;
> 
> being appended in output_constant.
> 

Yes, but that can only be done in the .merge.str section,
otherwise it would happen in structure initializers as well.
And I would like to undo the case when Ada programs do

Process ("ABCD" & Ascii.NUL);

but not for embedded NUL in the string constant.
like:

Process ("ABCD" & Acii.NUL & "EFGH");


Bernd.

Re: [PATCH] PR86844: Fix for store merging

[Richard Biener]

On Tue, Aug 7, 2018 at 1:35 PM Andreas Krebbel  wrote:
>
> From: Andreas Krebbel 
>
> Bootstrapped and regtested on s390x and x86_64.

Eric, didn't your patches explicitely handle this case of a non-constant
inbetween?  Can you have a look / review here?

Thanks,
Richard.

> gcc/ChangeLog:
>
> 2018-08-07  Andreas Krebbel  
>
> PR tree-optimization/86844
> * gimple-ssa-store-merging.c (check_no_overlap): Add a check to
> reject overlaps if it has seen a non-constant store in between.
>
> gcc/testsuite/ChangeLog:
>
> 2018-08-07  Andreas Krebbel  
>
> PR tree-optimization/86844
> * gcc.dg/pr86844.c: New test.
> ---
>  gcc/gimple-ssa-store-merging.c |  8 +++-
>  gcc/testsuite/gcc.dg/pr86844.c | 42 
> ++
>  2 files changed, 49 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.dg/pr86844.c
>
> diff --git a/gcc/gimple-ssa-store-merging.c b/gcc/gimple-ssa-store-merging.c
> index 0ae4581..2abef2e 100644
> --- a/gcc/gimple-ssa-store-merging.c
> +++ b/gcc/gimple-ssa-store-merging.c
> @@ -2401,13 +2401,19 @@ check_no_overlap (vec 
> m_store_info, unsigned int i,
>   unsigned HOST_WIDE_INT end)
>  {
>unsigned int len = m_store_info.length ();
> +  bool seen_group_end_store_p = false;
> +
>for (++i; i < len; ++i)
>  {
>store_immediate_info *info = m_store_info[i];
>if (info->bitpos >= end)
> break;
> +  if (info->rhs_code != INTEGER_CST)
> +   seen_group_end_store_p = true;
>if (info->order < last_order
> - && (rhs_code != INTEGER_CST || info->rhs_code != INTEGER_CST))
> + && (rhs_code != INTEGER_CST
> + || info->rhs_code != INTEGER_CST
> + || seen_group_end_store_p))
> return false;
>  }
>return true;
> diff --git a/gcc/testsuite/gcc.dg/pr86844.c b/gcc/testsuite/gcc.dg/pr86844.c
> new file mode 100644
> index 000..9ef08e9
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr86844.c
> @@ -0,0 +1,42 @@
> +/* { dg-do run } */
> +/* { dg-require-effective-target stdint_types } */
> +/* { dg-options "-O1 -fstore-merging" } */
> +
> +#include 
> +
> +struct foo
> +{
> +  union
> +  {
> +uint32_t u4i;
> +
> +struct
> +{
> +  uint8_t x;
> +  uint8_t y;
> +  uint8_t z;
> +  uint8_t w;
> +} s;
> +  } u;
> +  uint8_t v;
> +};
> +
> +void __attribute__((noinline,noclone))
> +f (struct foo *a)
> +{
> +  a->u.u4i = 0;
> +  a->u.s.w = 222;
> +  a->u.s.y = 129;
> +  a->u.s.z = a->v;
> +}
> +
> +int
> +main ()
> +{
> +  struct foo s;
> +
> +  f ();
> +
> +  if (s.u.s.w != 222)
> +__builtin_abort ();
> +}
> --
> 2.9.1
>

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

[Richard Biener]

On Fri, 17 Aug 2018, Bernd Edlinger wrote:

> On 08/17/18 14:19, Richard Biener wrote:
> > On Fri, 17 Aug 2018, Bernd Edlinger wrote:
> > 
> >> Richard Biener wrote:
> >>> +embedded @code{NUL} characters.  However, the
> >>> +@code{TREE_STRING_LENGTH} always includes a trailing @code{NUL} that
> >>> +is not part of the language string literal but appended by the front end.
> >>> +If the string shall not be @code{NUL}-terminated the @code{TREE_TYPE}
> >>> +is one character shorter than @code{TREE_STRING_LENGTH}.
> >>> +Excess caracters other than one trailing @code{NUL} character are not
> > 
> > characters btw.
> > 
> 
> thanks, updated.
> 
> > I read the above that the string literal for
> > 
> > char x[2] = "1";
> > 
> > is actually "1\0\0" - there's one NUL that is not part of the language
> > string literal.  The second sentence then suggests that both \0
> > are removed because 2 is less than 3?
> > 
> 
> maybe 2 is a bad example, lets consider:
> char x[20] = "1";
> 
> That is a string_cst with STRING_LENGTH = 2, content = "2\0\0"
> the array_type is used on both x, and the string_cst,
> I was assuming that both tree objects refer to the same type object.
> which is char[0..20-1]

Oh, didn't realize we use char[2] for the STRING_CST.  Makes
my suggestion to use char[] instead not (very) much worse than the
existing practice then.

> varasm assembles the bytes that are given by STRING_LENGTH
> and appends zeros as appropriate.
> 
> > As said, having this extra semantics of a STRING_CST tied to
> > another tree node (its TREE_TYPE) looks ugly.
> > 
> >>> +permitted.
> >>>
> >>> I find this very confusing and oppose to that change.  Can we get
> >>> back to the drawing board please?  If we want an easy way to
> >>> see whether a string is "properly" terminated then maybe we can
> >>> simply use a flag that gets set by build_string?
> >>>
> >>
> >> What I mean with that is the case like
> >> char x[2] = "123456";
> >>
> >> which is build_string(7, "123456"), but with a type char[2],
> >> so varasm throws away "3456\0".
> > 
> > I think varasm throws away chars not because of the type of
> > the STRING_CST but because of the available storage in x.
> > 
> 
> But at other places we look at the type of the string_cst, don't we?
> Shouldn't those be the same?

I think most (all?) places look at TREE_TYPE (TREE_TYPE (string))
only.  I'm not aware of users of the array domain of the array type
of a string - but I'm far from knowing GCC inside-out ;)

> >> I want to say that this is not okay, the excess precision
> >> should only be used to strip the nul termination, in cases
> >> where it is intended to be a assembled as a not zero terminated
> >> string.  But maybe the wording could be improved?
> > 
> > ISTR we always assemble a NUL in .strings to get string merging
> > working.
> > 
> 
> String merging is not working when the string is not explicitly
> NUL terminated, my followup patch here tries to fix that:
> 
> [PATCH] Handle not explicitly zero terminated strings in merge sections
> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00481.html

I'd have expected sth as simple as

  if (merge_strings && str[thissize - 1] != '\0')
thissize++;

being appended in output_constant.

Richard.

[PATCH] scev: dump final value replacement expressions

[Alexander Monakov]

Hello,

I'd like to apply the following patch to make GENERIC expressions
used in final value replacement visible in pass dumps.

We currently dump only the last statement, but that is not very
convenient as final values are sometimes complex and expand to
multiple gimple statements.

* tree-scalar-evolution.c (final_value_replacement_loop): Dump
full GENERIC expression used for replacement.

diff --git a/gcc/tree-scalar-evolution.c b/gcc/tree-scalar-evolution.c
index 69122f2652f..6475743a26a 100644
--- a/gcc/tree-scalar-evolution.c
+++ b/gcc/tree-scalar-evolution.c
@@ -3617,7 +3617,8 @@ final_value_replacement_loop (struct loop *loop)
{
  fprintf (dump_file, "\nfinal value replacement:\n  ");
  print_gimple_stmt (dump_file, phi, 0);
- fprintf (dump_file, "  with\n  ");
+ fprintf (dump_file, " with expr: ");
+ print_generic_expr (dump_file, def);
}
   def = unshare_expr (def);
   remove_phi_node (, false);
@@ -3656,6 +3657,7 @@ final_value_replacement_loop (struct loop *loop)
   gsi_insert_before (, ass, GSI_SAME_STMT);
   if (dump_file)
{
+ fprintf (dump_file, "\n final stmt:\n  ");
  print_gimple_stmt (dump_file, ass, 0);
  fprintf (dump_file, "\n");
}

[PATCH RFC] gimplefe: expose MULT_HIGHPART_EXPR

[Alexander Monakov]

For testing generic expansion of MULT_HIGHPART_EXPR I chose to expose it
via the GIMPLE FE with the following patch.

"h*" is how tree-pretty-print dumps MULT_HIGHPART_EXPR.

This patch accepts "h*" which ideally shouldn't happen, but
I don't see a simple way to fix that.

Is this desirable for trunk?

Is there general policy for how fancy tree codes should be exposed in
the GIMPLE FE?

* c/gimple-parser.c (c_parser_gimple_statement): Add "h*" for
MULT_HIGHPART_EXPR.

diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c
index 1be5d14dc2d..cf05c936166 100644
--- a/gcc/c/gimple-parser.c
+++ b/gcc/c/gimple-parser.c
@@ -450,6 +450,7 @@ c_parser_gimple_statement (c_parser *parser, gimple_seq 
*seq)
 
gimple-binary-expression:
  gimple-unary-expression * gimple-unary-expression
+ gimple-unary-expression h* gimple-unary-expression
  gimple-unary-expression / gimple-unary-expression
  gimple-unary-expression % gimple-unary-expression
  gimple-unary-expression + gimple-unary-expression
@@ -544,6 +545,18 @@ c_parser_gimple_binary_expression (c_parser *parser)
 case CPP_OR_OR:
   c_parser_error (parser, "%<||%> not valid in GIMPLE");
   return ret;
+case CPP_NAME:
+   {
+ tree id = c_parser_peek_token (parser)->value;
+ if (strcmp (IDENTIFIER_POINTER (id), "h") == 0
+ && c_parser_peek_2nd_token (parser)->type == CPP_MULT)
+   {
+ c_parser_consume_token (parser);
+ code = MULT_HIGHPART_EXPR;
+ break;
+   }
+   }
+  /* Fallthru.  */
 default:
   /* Not a binary expression.  */
   return lhs;

[PATCH RFC] add generic expansion for MULT_HIGHPART_EXP

[Alexander Monakov]

Hello,

We currently have an unfortunate situation where, on the one hand, scalar
MULT_HIGHPART_EXPR is usable only if the backend provides the corresponding
pattern (otherwise ICEs in expand), but on the other hand, the BRIG frontend
wants to make use of it.

I think BRIG FE is making assumptions on which types are going to work (based
on behavior of i386 backend), and this recently broke when the backend stopped
exposing SImode mult-highpart in 64-bit mode, causing PR 86948.

For scalar integer modes it is easy enough to implement generic expansion via
widening multiply (although it still won't work for the widest mode).

Do we want such functionality on trunk?

* expr.c (expand_expr_real_2) : Add generic
expansion via widening multiply and shift for integer modes.
* optabs.c (expand_mult_highpart): Receive 'method' from caller.
* optabs.h (expand_mult_highpart): Adjust prototype.

diff --git a/gcc/expr.c b/gcc/expr.c
index de6709defd6..b3e33077b3d 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -8913,10 +8913,23 @@ expand_expr_real_2 (sepops ops, rtx target, 
machine_mode tmode,
   goto binop;
 
 case MULT_HIGHPART_EXPR:
-  expand_operands (treeop0, treeop1, subtarget, , , EXPAND_NORMAL);
-  temp = expand_mult_highpart (mode, op0, op1, target, unsignedp);
-  gcc_assert (temp);
-  return temp;
+  {
+   int method = can_mult_highpart_p (mode, unsignedp);
+   if (!method)
+ {
+   gcc_assert (GET_MODE_CLASS (mode) == MODE_INT);
+   machine_mode wmode = GET_MODE_2XWIDER_MODE (mode).require ();
+   int prec = GET_MODE_UNIT_BITSIZE (mode);
+   ops->code = WIDEN_MULT_EXPR;
+   ops->type = build_nonstandard_integer_type (prec * 2, unsignedp);
+   temp = expand_expr_real_2 (ops, NULL_RTX, wmode, EXPAND_NORMAL);
+   temp = expand_shift (RSHIFT_EXPR, wmode, temp, prec, target,
+unsignedp);
+   return convert_modes (mode, wmode, temp, unsignedp);
+ }
+   expand_operands (treeop0, treeop1, subtarget, , , 
EXPAND_NORMAL);
+   return expand_mult_highpart (mode, op0, op1, target, unsignedp, method);
+  }
 
 case FIXED_CONVERT_EXPR:
   op0 = expand_normal (treeop0);
diff --git a/gcc/optabs.c b/gcc/optabs.c
index cadf4676c98..616d6f86720 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -5871,20 +5871,17 @@ expand_vec_cmp_expr (tree type, tree exp, rtx target)
 
 rtx
 expand_mult_highpart (machine_mode mode, rtx op0, rtx op1,
- rtx target, bool uns_p)
+ rtx target, bool uns_p, int method)
 {
   struct expand_operand eops[3];
   enum insn_code icode;
-  int method, i;
+  int i;
   machine_mode wmode;
   rtx m1, m2;
   optab tab1, tab2;
 
-  method = can_mult_highpart_p (mode, uns_p);
   switch (method)
 {
-case 0:
-  return NULL_RTX;
 case 1:
   tab1 = uns_p ? umul_highpart_optab : smul_highpart_optab;
   return expand_binop (mode, tab1, op0, op1, target, uns_p,
diff --git a/gcc/optabs.h b/gcc/optabs.h
index f9a8169daf8..ad78c4cbe76 100644
--- a/gcc/optabs.h
+++ b/gcc/optabs.h
@@ -316,7 +316,7 @@ extern rtx expand_vec_cond_expr (tree, tree, tree, tree, 
rtx);
 extern rtx expand_vec_series_expr (machine_mode, rtx, rtx, rtx);

 /* Generate code for MULT_HIGHPART_EXPR.  */
-extern rtx expand_mult_highpart (machine_mode, rtx, rtx, rtx, bool);
+extern rtx expand_mult_highpart (machine_mode, rtx, rtx, rtx, bool, int);

 extern rtx expand_sync_lock_test_and_set (rtx, rtx, rtx);
 extern rtx expand_atomic_test_and_set (rtx, rtx, enum memmodel);

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

[Bernd Edlinger]

On 08/17/18 14:19, Richard Biener wrote:
> On Fri, 17 Aug 2018, Bernd Edlinger wrote:
> 
>> Richard Biener wrote:
>>> +embedded @code{NUL} characters.  However, the
>>> +@code{TREE_STRING_LENGTH} always includes a trailing @code{NUL} that
>>> +is not part of the language string literal but appended by the front end.
>>> +If the string shall not be @code{NUL}-terminated the @code{TREE_TYPE}
>>> +is one character shorter than @code{TREE_STRING_LENGTH}.
>>> +Excess caracters other than one trailing @code{NUL} character are not
> 
> characters btw.
> 

thanks, updated.

> I read the above that the string literal for
> 
> char x[2] = "1";
> 
> is actually "1\0\0" - there's one NUL that is not part of the language
> string literal.  The second sentence then suggests that both \0
> are removed because 2 is less than 3?
> 

maybe 2 is a bad example, lets consider:
char x[20] = "1";

That is a string_cst with STRING_LENGTH = 2, content = "2\0\0"
the array_type is used on both x, and the string_cst,
I was assuming that both tree objects refer to the same type object.
which is char[0..20-1]

varasm assembles the bytes that are given by STRING_LENGTH
and appends zeros as appropriate.

> As said, having this extra semantics of a STRING_CST tied to
> another tree node (its TREE_TYPE) looks ugly.
> 
>>> +permitted.
>>>
>>> I find this very confusing and oppose to that change.  Can we get
>>> back to the drawing board please?  If we want an easy way to
>>> see whether a string is "properly" terminated then maybe we can
>>> simply use a flag that gets set by build_string?
>>>
>>
>> What I mean with that is the case like
>> char x[2] = "123456";
>>
>> which is build_string(7, "123456"), but with a type char[2],
>> so varasm throws away "3456\0".
> 
> I think varasm throws away chars not because of the type of
> the STRING_CST but because of the available storage in x.
> 

But at other places we look at the type of the string_cst, don't we?
Shouldn't those be the same?

>> I want to say that this is not okay, the excess precision
>> should only be used to strip the nul termination, in cases
>> where it is intended to be a assembled as a not zero terminated
>> string.  But maybe the wording could be improved?
> 
> ISTR we always assemble a NUL in .strings to get string merging
> working.
> 

String merging is not working when the string is not explicitly
NUL terminated, my followup patch here tries to fix that:

[PATCH] Handle not explicitly zero terminated strings in merge sections
https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00481.html


Bernd.

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

[Richard Biener]

On Fri, 17 Aug 2018, Bernd Edlinger wrote:

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

characters btw.

I read the above that the string literal for

char x[2] = "1";

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

As said, having this extra semantics of a STRING_CST tied to
another tree node (its TREE_TYPE) looks ugly.

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

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

> I want to say that this is not okay, the excess precision
> should only be used to strip the nul termination, in cases
> where it is intended to be a assembled as a not zero terminated
> string.  But maybe the wording could be improved?

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

Richard.

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

[Joseph Myers]

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.

There's a difference between explicitly not handling it and silently 
passing a wrong value.

> In theory something like WCHAR_TYPE_SIZE / BITS_PER_UNIT probably does
> the trick.   I'm a bit leery of using that though.  We don't use it
> anywhere else within GCC AFAICT.

WCHAR_TYPE_SIZE is wrong because it doesn't account for flag_short_wchar.  
As far as I can see only ada/gcc-interface/targtyps.c uses WCHAR_TYPE_SIZE 
now.  TYPE_PRECISION (wchar_type_node) / BITS_PER_UNIT is what should be 
used.

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

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

[Richard Biener]

On Fri, 17 Aug 2018, Bernd Edlinger wrote:

> Richard Biener wrote:
> 
> > Note that I'm a little bit confused here given build_string
> > already appends a '\0' after TREE_STRING_LENGTH.  So it is safe
> > to call strlen() on all STRING_CSTs.  I think I raised this in
> > the review of one of Bernds patches but to be honest all the
> > various threads have become a bit convoluted (esp. if joining
> > in after two weeks of vacation).
> >
> > So -- why is what we do currently not enough?
> >
> > Quoting our single STRING_CST creator:
> >
> > tree
> > build_string (int len, const char *str)
> > {
> >   tree s;
> >   size_t length;
> >
> >  /* Do not waste bytes provided by padding of struct tree_string.  */
> >  length = len + offsetof (struct tree_string, str) + 1;
> >
> >  record_node_allocation_statistics (STRING_CST, length);
> >
> >  s = (tree) ggc_internal_alloc (length);
> >
> >  memset (s, 0, sizeof (struct tree_typed));
> >  TREE_SET_CODE (s, STRING_CST);
> >  TREE_CONSTANT (s) = 1;
> >  TREE_STRING_LENGTH (s) = len;
> >  memcpy (s->string.str, str, len);
> >  s->string.str[len] = '\0';
> >
> >  return s;
> >}
> 
> Thanks for this question.
> 
> I have two issues with that.
> 1. it adds not a wide character nul, only a single byte '\0'.
> 2. the '\0' here is _guaranteed_ not to be assembled
> by varasm.
>Since it is at TREE_STRING_LENGTH+1.
> 
> That is fine for some string constants, like ASM constaraints.
> it makes most of the time sense, as long as it is not
> used for folding of memxxx functions.
> 
> Of course the whole patch series is dependent on the
> consensus about how we want to handle string constants
> in the middle-end, so no problem with going back to the
> drawing board, that's the reason why I did not apply the
> already approved bits, and I guess we are not in a hurry.
> 
> What I see, is that string constants are most of the time
> handled like the C language strings, that is there
> are different character wides, and the array_type on the
> string constants, includes another NUL (wide) char which
> is assembled along with the rest of the bytes, but it is
> not written in the language string constant.
> The front end appends this before giving the string
> constant to build_string.
> That means at least most of the time.
> 
> So I thought it might be good to have some more
> easily checkable things regarding the zero termiation,
> or what is allowed for excess precision.
> 
> It's possible that this shoots over the target, but I think
> this hardening in the varasm is of some value.
> 
> This way how this patch works has one advantage:
> That it is easy to check for "there must be one wide char zero",
> if it is someting that can't be checked, like:
> "there may be a nul or not", then it is impossible to be checked.

What I most object to is the fact that this makes the requirement
of TREE_TYPE of a STRING_CST to be an ARRAY_TYPE a hard one
while the ARRAY_TYPE doesn't add any useful information.  Those
array types are quite heavy structures while the only thing
we really need is the type of the elements.  We could improve
memory use greatly here by using charT[] as TREE_TYPE of
STRING_CSTs.

Note that not all STRING_CSTs do get a type (probably all that
end up in the IL do).

So, why not add a TREE_STRING_NUL_TERMINATED flag instad that
is true when the string is NUL-terminated within TREE_STRING_LENGTH
and is false when we do not know (to make the transition easy).

Richard.

> Well yeah, it's an idea.
> 
> 
> 
> Bernd.
> 
> 

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

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

[Bernd Edlinger]

On 08/17/18 06:46, Jeff Law wrote:
> On 08/05/2018 04:28 AM, Bernd Edlinger wrote:
>> Hi,
>>
>> I would like to do a minor tweak to the patch.
>> While staring at the other patch I realized that I should
>> better pass size and not thissize to the check
>> function, instead of making use of how thissize is
>> computed using MIN above.  This means that this condition
>>
>> +  if ((unsigned)len != size && (unsigned)len != size + elts)
>> +return false;
>>
>> should better and more readable be:
>>
>> +  if (size < (unsigned)len && size != len - elts)
>> +return false;
>>
>> Furthermore I wanted to have the check function static,
>> which I missed to do previously.
>>
>> For completeness, these are again the precondition patches
>> for this patch:
>>
>> [PATCH] Handle overlength strings in the C FE
>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00142.html
> I believe Joseph ack'd this already:
> 
> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00655.html
> 
> 

Yes.

>>
>> [PATCH] Make GO string literals properly NUL terminated
>> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01931.html
> Ian didn't object, deferring to the larger scoped review.  My
> understanding is this should be a nop for Go.  Given it takes us closer
> to have a canonical form, I'll go ahead and ACK for the trunk.
> 
> 

Yes, it should be a no-op, eventually the strings will go
into the string merge section, with the follow-up patch.
Everything works nicely, as I always bootstrap/regtest with GO.


>>
>> [PATCH] [Ada] Make middle-end string literals NUL terminated
>> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01929.html
> This is OK.
> 
> 
>>
>> [PATCH] Create internally nul terminated string literals in fortan FE
>> https://gcc.gnu.org/ml/fortran/2018-08/msg0.html
> This is OK too.  THough it is unclear why we're not fixing
> gfc_build_string_const vs introducing gfc_build_hollerith_string_const.
> Are hollerith strings naturally not terminated in the fortran front-end
> and thus need special handling, while other calls to
> gfc_build_string_const are always with naturally nul terminated strings?
> 
> 

I think all other strings are null terminated, except the hollerith strings.

> 
> 
>>
>>
>> Bootstrapped, as usual.
>> Is it OK for trunk?
> So as a toplevel comment.While there may be some interaction with
> Martin's code to detect and warn for unterminated strings passed to
> strlen and other cases, I think the overall goal of canonicalizing our
> internal representation of STRING_CST along with a verification step is
> a good thing and it may make some of Martin's work easier.
> 
> At this point are the prereqs of the varasm verification patch all ACK'd?
> 

Yes, there is a JIT code gen bug, that was caught by the assertion in my
patch:

[PATCH] Fix not properly nul-terminated string constants in JIT
https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00370.html


That is a simple bug, which means, that the JIT FE was never able to
assemble strings > 200 character length.

Fixing a bug like that makes me I wonder, if there is any test coverage
for JIT except the gcc test suite itself.


Thanks
Bernd.

Re: [PATCH][GCC][AARCH64] Use stdint integers in vect_su_add_sub.c

[Richard Biener]

On Fri, Aug 3, 2018 at 6:28 PM Matthew Malcomson
 wrote:
>
> On 02/08/18 20:18, James Greenhalgh wrote:
> > On Tue, Jul 31, 2018 at 04:53:19AM -0500, Matthew Malcomson wrote:
> >> Fixing the ilp32 issue that Christophe found.
> >>
> >> The existing testcase uses `long` to represent a 64 bit integer.
> >> This breaks when compiled using the `-mabi=ilp32` flag.
> >> We switch the use of int/long for int32_t/int64_t to avoid this problem
> >> and show the requirement of a widening operation more clearly.
> > Normally we try to avoid pulling more headers than we need in the testsuite.
> >
> > Have you seen this construct:
> >
> >typedef unsigned __attribute__((mode(DI))) uint64_t;
> >
> > It can be a good way to ensure you have the right type for the patterns you
> > are adding.

You can also simply use __UINT64_TYPE__

Use

echo | gcc -dD -E -

to see what GCC predefines.

Richard.

> > Thanks,
> > James
> Thanks! No I hadn't seen that construct before.
> Have attached a patch using that construct.
>
> Matthew

[PATCH] Move cpp_macro to cpplib.h

[Nathan Sidwell]

Way back Geoff Keating broke cpp-id-data.h out of internal.h so that PCH 
could write out tokenized macro definitions.  That removed the need to 
stringize and retokenize them.  cpp-id-data.h was fed to the gty 
machinery, and users of cpplib continued to use cpplib.h unaware of the 
cpp_macro definition.  but some of its effects are felt in cpplib.h (the 
GTY markup for instance).


Since then we now include cpp-id-data.h in a few places where we need to 
look inside a cpp_macro.  That's a nasty hole punched through the 
abstraction layer :(


This patch moves a couple of cpplib-internal declarations to internal.h 
and moves cpp_macro to cpplib.h, thereby exposing it to users properly. 
The remains of cpp-id-data.h will evaporate with this patch series, but 
we're not there yet.


This allows the includers of cpp-id-data.h to become better citizens and 
use cpplib.h.  I'll be morphing the cpp_macro definition, but thought it 
better to move then morph, than vice-versa.


booted & tested on x86_64-linux

nathan
--
Nathan Sidwell
2018-08-17  Nathan Sidwell  

	libcpp/
	* cpp-id-data.h (uchar, UC): Move to internal.h
	(struct cpp_macro): Move to cpplib.h.
	* internal.h (uchar, UC): From cpp-id-data.h.
	* include/cpplib.h (struct cpp_macro): From cpp-id-data.h.
	gcc/c-family/
	* c-ada-spec.c: Don't #include "cpp-id-data.h"
	* c-cppbuiltin.c: Likewise.
	gcc/
	* cppbuiltin.c: Include "cpplib.h", not "cpp-id-data.h".

Index: gcc/c-family/c-ada-spec.c
===
--- gcc/c-family/c-ada-spec.c	(revision 263587)
+++ gcc/c-family/c-ada-spec.c	(working copy)
@@ -27,7 +27,6 @@ along with GCC; see the file COPYING3.
 #include "c-ada-spec.h"
 #include "fold-const.h"
 #include "c-pragma.h"
-#include "cpp-id-data.h"
 #include "stringpool.h"
 #include "attribs.h"
 
Index: gcc/c-family/c-cppbuiltin.c
===
--- gcc/c-family/c-cppbuiltin.c	(revision 263587)
+++ gcc/c-family/c-cppbuiltin.c	(working copy)
@@ -31,7 +31,6 @@ along with GCC; see the file COPYING3.
 #include "output.h"		/* For user_label_prefix.  */
 #include "debug.h"		/* For dwarf2out_do_cfi_asm.  */
 #include "common/common-target.h"
-#include "cpp-id-data.h"
 #include "cppbuiltin.h"
 
 #ifndef TARGET_OS_CPP_BUILTINS
Index: gcc/cppbuiltin.c
===
--- gcc/cppbuiltin.c	(revision 263587)
+++ gcc/cppbuiltin.c	(working copy)
@@ -25,7 +25,7 @@ along with GCC; see the file COPYING3.
 #include "tree.h"
 #include "version.h"
 #include "flags.h"
-#include "cpp-id-data.h"
+#include "cpplib.h"
 #include "cppbuiltin.h"
 
 
Index: libcpp/include/cpp-id-data.h
===
--- libcpp/include/cpp-id-data.h	(revision 263587)
+++ libcpp/include/cpp-id-data.h	(working copy)
@@ -17,12 +17,6 @@ along with this program; see the file CO
 
 #include "cpplib.h"
 
-#if !defined (HAVE_UCHAR) && !defined (IN_GCC)
-typedef unsigned char uchar;
-#endif
-
-#define UC (const unsigned char *)  /* Intended use: UC"string" */
-
 /* Chained list of answers to an assertion.  */
 struct GTY(()) answer {
   struct answer *next;
@@ -30,53 +24,3 @@ struct GTY(()) answer {
   cpp_token GTY ((length ("%h.count"))) first[1];
 };
 
-/* Each macro definition is recorded in a cpp_macro structure.
-   Variadic macros cannot occur with traditional cpp.  */
-struct GTY(()) cpp_macro {
-  /* Parameters, if any.  If parameter names use extended identifiers,
- the original spelling of those identifiers, not the canonical
- UTF-8 spelling, goes here.  */
-  cpp_hashnode ** GTY ((nested_ptr (union tree_node,
-		"%h ? CPP_HASHNODE (GCC_IDENT_TO_HT_IDENT (%h)) : NULL",
-			"%h ? HT_IDENT_TO_GCC_IDENT (HT_NODE (%h)) : NULL"),
-			length ("%h.paramc")))
-params;
-
-  /* Replacement tokens (ISO) or replacement text (traditional).  See
- comment at top of cpptrad.c for how traditional function-like
- macros are encoded.  */
-  union cpp_macro_u
-  {
-cpp_token * GTY ((tag ("0"), length ("%0.count"))) tokens;
-const unsigned char * GTY ((tag ("1"))) text;
-  } GTY ((desc ("%1.traditional"))) exp;
-
-  /* Definition line number.  */
-  source_location line;
-
-  /* Number of tokens in expansion, or bytes for traditional macros.  */
-  unsigned int count;
-
-  /* Number of parameters.  */
-  unsigned short paramc;
-
-  /* If a function-like macro.  */
-  unsigned int fun_like : 1;
-
-  /* If a variadic macro.  */
-  unsigned int variadic : 1;
-
-  /* If macro defined in system header.  */
-  unsigned int syshdr   : 1;
-
-  /* Nonzero if it has been expanded or had its existence tested.  */
-  unsigned int used : 1;
-
-  /* Indicate which field of 'exp' is in use.  */
-  unsigned int traditional : 1;
-
-  /* Indicate whether the tokens include extra CPP_PASTE tokens at the
- end to track invalid redefinitions with consecutive CPP_PASTE
- tokens.  */
-  

[PATCH] Call braced_list_to_string after array size is fixed

[Bernd Edlinger]

Hi,

Yes I know Martin will see this as a capital offense, but to my excuse I must 
say
it only happened in self defense :-)

As my other patch series depends on STRNG_CSTs with certain properties, it got
broken by the way the string constants are created now, due to the 71625 patch.

In order to be able to bootstrap my patch with current trunk, I had to move
the point where the char array constructor is converted to a STRING_CST to the
point directly after the array bounds are fixed, so that it is possible to 
create
the string constant (with implicit NUL termination) like all other C strings, 
without
making the array larger.

I added a test case for a crash with an invalid string intializer (in C++).
and removed an xfail in the new test case string2.C.  This way the C++ 
diagnostics
matches the state before 71625 was applied.

This does not fix the other problems with 71625, but I don't think they are 
related
to the way the string constants are created, so there should be no conflict.


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


Thanks
Bernd.
c-family:
2018-08-17  Bernd Edlinger  

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

c:
2018-08-17  Bernd Edlinger  

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


cp:
2018-08-17  Bernd Edlinger  

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

testsuite:
2018-08-17  Bernd Edlinger  

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


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

[PATCH][8-branch][OBVIOUS] Fix wrong option declaration of fcilkplus (PR other/86992).

[Martin Liška]

Hi.

This is obvious fix of wrong option declaration. Unfortunately current
AWK can't catch that.

I'm going to install that to GCC-8 branch, it's the only affected now.
Trunk fix is the same, but it's part of an overhaul.

Martin

gcc/c-family/ChangeLog:

2018-08-17  Martin Liska  

PR other/86992
* c.opt: Fix declaration of cilkplus.
---
 gcc/c-family/c.opt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index a4c8c8ffcb3..f591b39be5a 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1339,8 +1339,8 @@ Driver
 static-libmpxwrappers
 Driver
 
-fcilkplus Undocumented
-C ObjC C++ ObjC++ LTO Report Var(flag_cilkplus) Init(0)
+fcilkplus
+C ObjC C++ ObjC++ LTO Report Var(flag_cilkplus) Init(0) Undocumented
 Deprecated in GCC 8.  This switch has no effect.
 
 fconcepts

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

[Bernd Edlinger]

Richard Biener wrote:
> +embedded @code{NUL} characters.  However, the
> +@code{TREE_STRING_LENGTH} always includes a trailing @code{NUL} that
> +is not part of the language string literal but appended by the front end.
> +If the string shall not be @code{NUL}-terminated the @code{TREE_TYPE}
> +is one character shorter than @code{TREE_STRING_LENGTH}.
> +Excess caracters other than one trailing @code{NUL} character are not
> +permitted.
> 
> I find this very confusing and oppose to that change.  Can we get
> back to the drawing board please?  If we want an easy way to
> see whether a string is "properly" terminated then maybe we can
> simply use a flag that gets set by build_string?
>

What I mean with that is the case like
char x[2] = "123456";

which is build_string(7, "123456"), but with a type char[2],
so varasm throws away "3456\0".

I want to say that this is not okay, the excess precision
should only be used to strip the nul termination, in cases
where it is intended to be a assembled as a not zero terminated
string.  But maybe the wording could be improved?


Bernd.

Re: [PATCH] Make strlen range computations more conservative

[Richard Biener]

On Tue, 7 Aug 2018, Martin Sebor wrote:

> On 08/07/2018 11:44 AM, Richard Biener wrote:
> > On August 7, 2018 4:37:00 PM GMT+02:00, Martin Sebor 
> > wrote:
> > > On 08/07/2018 02:51 AM, Richard Biener wrote:
> > > > On August 7, 2018 4:24:42 AM GMT+02:00, Martin Sebor
> > >  wrote:
> > > > > On 07/26/2018 02:55 AM, Richard Biener wrote:
> > > > > > On Wed, 25 Jul 2018, Martin Sebor wrote:
> > > > > > 
> > > > > > > > BUT - for the string_constant and c_strlen functions we are,
> > > > > > > > in all cases we return something interesting, able to look
> > > > > > > > at an initializer which then determines that type.  Hopefully.
> > > > > > > > I think the strlen() folding code when it sets SSA ranges
> > > > > > > > now looks at types ...?
> > > > > > > > 
> > > > > > > > Consider
> > > > > > > > 
> > > > > > > > struct X { int i; char c[4]; int j;};
> > > > > > > > struct Y { char c[16]; };
> > > > > > > > 
> > > > > > > > void foo (struct X *p, struct Y *q)
> > > > > > > > {
> > > > > > > >   memcpy (p, q, sizeof (struct Y));
> > > > > > > >   if (strlen ((char *)(struct Y *)p + 4) < 7)
> > > > > > > > abort ();
> > > > > > > > }
> > > > > > > > 
> > > > > > > > here the GIMPLE IL looks like
> > > > > > > > 
> > > > > > > >   const char * _1;
> > > > > > > > 
> > > > > > > >[local count: 1073741825]:
> > > > > > > >   _5 = MEM[(char * {ref-all})q_4(D)];
> > > > > > > >   MEM[(char * {ref-all})p_6(D)] = _5;
> > > > > > > >   _1 = p_6(D) + 4;
> > > > > > > >   _2 = __builtin_strlen (_1);
> > > > > > > > 
> > > > > > > > and I guess Martin would argue that since p is of type struct X
> > > > > > > > + 4 gets you to c[4] and thus strlen of that cannot be larger
> > > > > > > > than 3.  But of course the middle-end doesn't work like that
> > > > > > > > and luckily we do not try to draw such conclusions or we
> > > > > > > > are somehow lucky that for the testcase as written above we do
> > > not
> > > > > > > > (I'm not sure whether Martins changes in this area would derive
> > > > > > > > such conclusions in principle).
> > > > > > > 
> > > > > > > Only if the strlen argument were p->c.
> > > > > > > 
> > > > > > > > NOTE - we do not know the dynamic type here since we do not know
> > > > > > > > the dynamic type of the memory pointed-to by q!  We can only
> > > > > > > > derive that at q+4 there must be some object that we can
> > > > > > > > validly call strlen on (where Martin again thinks strlen
> > > > > > > > imposes constrains that memchr does not - sth I do not agree
> > > > > > > > with from a QOI perspective)
> > > > > > > 
> > > > > > > The dynamic type is a murky area.
> > > > > > 
> > > > > > It's well-specified in the middle-end.  A store changes the
> > > > > > dynamic type of the stored-to object.  If that type is
> > > > > > compatible with the surrounding objects dynamic type that one
> > > > > > is not affected, if not then the surrounding objects dynamic
> > > > > > type becomes unspecified.  There is TYPE_TYPELESS_STORAGE
> > > > > > to somewhat control "compatibility" of subobjects.
> > > > > 
> > > > > I never responded to this.  Using a dynamic (effective) type as
> > > > > you describe it would invalidate the aggressive loop optimization
> > > > > in the following:
> > > > > 
> > > > >   void foo (struct X *p)
> > > > >   {
> > > > >   struct Y y = { "12345678" };
> > > > >   memcpy (p, , sizeof (struct Y));
> > > > > 
> > > > >   // *p effective type is now struct Y
> > > > > 
> > > > >   int n = 0;
> > > > >   while (p->c[n])
> > > > > ++n;
> > > > > 
> > > > >   if (n < 7)
> > > > > abort ();
> > > > >   }
> > > > > 
> > > > > GCC unconditionally aborts, just as it does with strlen(p->c).
> > > > > Why is that not wrong (in either case)?
> > > > > 
> > > > > Because the code is invalid either way, for two reasons:
> > > > 
> > > > No, because the storage has only 4 non-null characters starting at
> > > offset 4?
> > > 
> > > No, for the reasons below.  I made a mistake of making
> > > the initializer string too short.  If we make it longer it
> > > still aborts.  Say with this
> > > 
> > >   struct Y y = { "123456789012345" };
> > > 
> > > we end up with this DCE:
> > > 
> > >  struct Y y;
> > > 
> > >:
> > >   MEM[(char * {ref-all})p_6(D)] = 0x353433323130393837363534333231;
> > >   __builtin_abort ();
> > > 
> > > With -fdump-tree-cddce1-details (and a patch to show the upper
> > > bound) we see:
> > > 
> > >   Found loop 1 to be finite: upper bound found: 3.
> > > 
> > > With -fno-aggressive-loop-optimizations the abort becomes
> > > conditional because the array bound isn't considered. I would
> > > expect you to know this since you implemented the feature.
> > 
> > Honza added the array indexing part and it may very well be too aggressive.
> > I have to take a closer look after vacation to tell. Can you open a PR and
> > CC me there?
> 
> I opened bug 86884.

Now that I returned from vacation the testcase is simply 

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

[Bernd Edlinger]

Richard Biener wrote:

> Note that I'm a little bit confused here given build_string
> already appends a '\0' after TREE_STRING_LENGTH.  So it is safe
> to call strlen() on all STRING_CSTs.  I think I raised this in
> the review of one of Bernds patches but to be honest all the
> various threads have become a bit convoluted (esp. if joining
> in after two weeks of vacation).
>
> So -- why is what we do currently not enough?
>
> Quoting our single STRING_CST creator:
>
> tree
> build_string (int len, const char *str)
> {
>   tree s;
>   size_t length;
>
>  /* Do not waste bytes provided by padding of struct tree_string.  */
>  length = len + offsetof (struct tree_string, str) + 1;
>
>  record_node_allocation_statistics (STRING_CST, length);
>
>  s = (tree) ggc_internal_alloc (length);
>
>  memset (s, 0, sizeof (struct tree_typed));
>  TREE_SET_CODE (s, STRING_CST);
>  TREE_CONSTANT (s) = 1;
>  TREE_STRING_LENGTH (s) = len;
>  memcpy (s->string.str, str, len);
>  s->string.str[len] = '\0';
>
>  return s;
>}

Thanks for this question.

I have two issues with that.
1. it adds not a wide character nul, only a single byte '\0'.
2. the '\0' here is _guaranteed_ not to be assembled
by varasm.
   Since it is at TREE_STRING_LENGTH+1.

That is fine for some string constants, like ASM constaraints.
it makes most of the time sense, as long as it is not
used for folding of memxxx functions.

Of course the whole patch series is dependent on the
consensus about how we want to handle string constants
in the middle-end, so no problem with going back to the
drawing board, that's the reason why I did not apply the
already approved bits, and I guess we are not in a hurry.

What I see, is that string constants are most of the time
handled like the C language strings, that is there
are different character wides, and the array_type on the
string constants, includes another NUL (wide) char which
is assembled along with the rest of the bytes, but it is
not written in the language string constant.
The front end appends this before giving the string
constant to build_string.
That means at least most of the time.

So I thought it might be good to have some more
easily checkable things regarding the zero termiation,
or what is allowed for excess precision.

It's possible that this shoots over the target, but I think
this hardening in the varasm is of some value.

This way how this patch works has one advantage:
That it is easy to check for "there must be one wide char zero",
if it is someting that can't be checked, like:
"there may be a nul or not", then it is impossible to be checked.

Well yeah, it's an idea.



Bernd.

[RFC][debug] Fix handling of vlas in lto

[Tom de Vries]

Hi,

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.

Any comments?

Is the approach in this patch ok, given that this approach works without
requiring a gdb with DW_OP_GNU_variable_value support?

Thanks,
- Tom

[debug] Fix handling of vlas in lto

2018-08-17  Tom de Vries  

* dwarf2out.c (dw_loc_list_1): Improve handling of want_address == 1.
(add_scalar_info): Try harder to add an exprloc for DW_AT_upper_bound.
Otherwise, try to add a DW_AT_location to the referenced die.

---
 gcc/dwarf2out.c | 21 -
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 8c6b4372874..408c3fda8cd 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -16870,7 +16870,7 @@ dw_loc_list_1 (tree loc, rtx varloc, int want_address,
   if (!descr)
 return 0;
 
-  if (want_address == 2 && !have_address
+  if (want_address != 0 && !have_address
   && (dwarf_version >= 4 || !dwarf_strict))
 {
   if (int_size_in_bytes (TREE_TYPE (loc)) > DWARF2_ADDR_SIZE)
@@ -20702,6 +20702,25 @@ add_scalar_info (dw_die_ref die, enum dwarf_attribute 
attr, tree value,
 later parameter.  */
  if (decl_die != NULL)
{
+ if (attr == DW_AT_upper_bound)
+   {
+ list = loc_list_from_tree (decl, 1, context);
+ if (list != NULL)
+   {
+ if (single_element_loc_list_p (list)
+ && (forms & dw_scalar_form_exprloc) != 0)
+   {
+ list = loc_list_from_tree (decl, 0, context);
+ add_AT_loc (die, attr, list->expr);
+ return;
+   }
+
+ if (!get_AT (decl_die, DW_AT_location))
+   add_AT_location_description (decl_die, DW_AT_location,
+list);
+   }
+   }
+
  add_AT_die_ref (die, attr, decl_die);
  return;
}

Re: [PATCH][2/4] Add rev_post_order_and_mark_dfs_back_seme

[Richard Biener]

On Sat, 4 Aug 2018, Bernhard Reutner-Fischer wrote:

> On Wed, 1 Aug 2018 at 16:31, Richard Biener  wrote:
> > --- a/gcc/cfganal.c
> > +++ b/gcc/cfganal.c
> > @@ -1057,6 +1057,119 @@ pre_and_rev_post_order_compute (int *pre_order, int 
> > *rev_post_order,
> >return pre_order_num;
> >  }
> >
> > +/* Unline pre_and_rev_post_order_compute we fill rev_post_order backwards
> 
> Unlike
> 
> > +   so iterating in RPO order needs to start with rev_post_order[n - 1]
> > +   going to rev_post_order[0].  If FOR_ITERATION is true then try to
> > +   make CFG cycles fit into small contiguous regions of the RPO order.
> > +   When FOR_ITERATION is true this requires up-to-date loop structures.  */
> > +
> > +int
> > +rev_post_order_and_mark_dfs_back_seme (struct function *fn, edge entry,
> > +  bitmap exit_bbs, bool for_iteration,
> > +  int *rev_post_order)
> > +{
> > +  int pre_order_num = 0;
> > +  int rev_post_order_num = 0;
> > +
> > +  /* Allocate stack for back-tracking up CFG.  Worst case we need
> > + O(n^2) edges but the following should suffice in practice without
> > + a need to re-allocate.  */
> > +  auto_vec stack (2 * n_basic_blocks_for_fn (fn));
> > +
> > +  int *pre = XNEWVEC (int, 2 * last_basic_block_for_fn (fn));
> > +  int *post = pre + last_basic_block_for_fn (fn);
> > +
> > +  /* BB flag to track nodes that have been visited.  */
> > +  auto_bb_flag visited (fn);
> > +  /* BB flag to track which nodes whave post[] assigned to avoid
> > + zeroing post.  */
> 
> s/whave/have/.
> 
> > +  free (pre);
> 
> XDELETEVEC (pre);
> 
> > +
> > +  /* Clear the temporarily allocated flags.  */
> > +  for (int i = 0; i < rev_post_order_num; ++i)
> > +BASIC_BLOCK_FOR_FN (fn, rev_post_order[i])->flags
> > +  &= ~(post_assigned|visited);
> > +
> > +  return rev_post_order_num;
> > +}
> > +
> > +
> > +
> >  /* Compute the depth first search order on the _reverse_ graph and
> > store in the array DFS_ORDER, marking the nodes visited in VISITED.
> 
> s/store/store it/
> 
> thanks,

Fixed.

Thanks,
Richard.

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

[Richard Biener]

On Sat, 4 Aug 2018, Bernd Edlinger wrote:

> On 08/03/18 23:36, Jeff Law wrote:
> > On 08/01/2018 05:35 AM, Bernd Edlinger wrote:
> >> Hi,
> >>
> >> this completes the previous patches, and adds a check in varasm.c
> >> that ensures that all string constants are NUL terminated,
> >> And that varasm does not strip anything but _exactly_ one NUL
> >> character.
> >>
> >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> >> Is it OK for trunk?
> >>
> >>
> >> Thanks
> >> Bernd.
> >>
> >>
> >> patch-varasm.diff
> >>
> >>
> >> 2018-08-01  Bernd Edlinger  
> >>
> >>* varasm.c (check_string_literal): New checking function.
> >>(output_constant): Use it.
> > My biggest concern here is that we've explicitly defined STRING_CST
> > nodes as not needing NUL termination.  See generic.texi.  That explicit
> > definition is almost certainly derived from existing practice that long
> > pre-dates the introduction of gimple/generic.
> > 
> > Even so I'm generally OK with the concept of tightening the rules here.
> > If we need to fault in more fixes, that's fine with me.  I'm assuming
> > that you and Ian have sorted out what to do WRT Go.
> > 
> > If we decide to go forward, you'll definitely want to update the
> > STRING_CST documentation in generic.texi.
> > 
> 
> This is the same patch with STRING_CST documentation updated.

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

I find this very confusing and oppose to that change.  Can we get
back to the drawing board please?  If we want an easy way to
see whether a string is "properly" terminated then maybe we can
simply use a flag that gets set by build_string?

Richard.

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

[Richard Biener]

On Fri, 3 Aug 2018, Jeff Law wrote:

> On 08/01/2018 05:35 AM, Bernd Edlinger wrote:
> > Hi,
> > 
> > this completes the previous patches, and adds a check in varasm.c
> > that ensures that all string constants are NUL terminated,
> > And that varasm does not strip anything but _exactly_ one NUL
> > character.
> > 
> > Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> > Is it OK for trunk?
> > 
> > 
> > Thanks
> > Bernd.
> > 
> > 
> > patch-varasm.diff
> > 
> > 
> > 2018-08-01  Bernd Edlinger  
> > 
> > * varasm.c (check_string_literal): New checking function.
> > (output_constant): Use it.
> My biggest concern here is that we've explicitly defined STRING_CST
> nodes as not needing NUL termination.  See generic.texi.  That explicit
> definition is almost certainly derived from existing practice that long
> pre-dates the introduction of gimple/generic.
> 
> Even so I'm generally OK with the concept of tightening the rules here.
> If we need to fault in more fixes, that's fine with me.  I'm assuming
> that you and Ian have sorted out what to do WRT Go.
> 
> If we decide to go forward, you'll definitely want to update the
> STRING_CST documentation in generic.texi.

Note that I'm a little bit confused here given build_string
already appends a '\0' after TREE_STRING_LENGTH.  So it is safe
to call strlen() on all STRING_CSTs.  I think I raised this in
the review of one of Bernds patches but to be honest all the
various threads have become a bit convoluted (esp. if joining
in after two weeks of vacation).

So -- why is what we do currently not enough?

Quoting our single STRING_CST creator:

tree
build_string (int len, const char *str)
{
  tree s;
  size_t length;

  /* Do not waste bytes provided by padding of struct tree_string.  */
  length = len + offsetof (struct tree_string, str) + 1;

  record_node_allocation_statistics (STRING_CST, length);

  s = (tree) ggc_internal_alloc (length);

  memset (s, 0, sizeof (struct tree_typed));
  TREE_SET_CODE (s, STRING_CST);
  TREE_CONSTANT (s) = 1;
  TREE_STRING_LENGTH (s) = len;
  memcpy (s->string.str, str, len);
  s->string.str[len] = '\0';

  return s;
}


Richard.

[PATCH] Fix PR86841

[Richard Biener]

Applied as obvious.

Richard.

2018-08-17  Richard Biener  

PR tree-optimization/86841
* wide-int-range.cc (wide_int_range_lshift): Use to_uhwi.

Index: gcc/wide-int-range.cc
===
--- gcc/wide-int-range.cc   (revision 263613)
+++ gcc/wide-int-range.cc   (working copy)
@@ -323,7 +323,7 @@ wide_int_range_lshift (wide_int _lb,
   /* Transform left shifts by constants into multiplies.  */
   if (wi::eq_p (vr1_lb, vr1_ub))
 {
-  int shift = wi::extract_uhwi (vr1_ub, 0, vr1_ub.get_precision ());
+  unsigned shift = vr1_ub.to_uhwi ();
   wide_int tmp = wi::set_bit_in_zero (shift, prec);
   return wide_int_range_multiplicative_op (res_lb, res_ub,
   MULT_EXPR, sign, prec,

Re: [PING] [PATCH] Fix wrong code with truncated string literals (PR 86711/86714)

[Bernd Edlinger]

Hi!


After the other patch has been applied, I re-based this patch accordingly.

Except the mechanical changes, there are a few notable differences to the
previous version:

In string_constant, I added a similar check for the STRING_CSTs
because when callers don't use mem_size, they assume to be
able to read "TREE_STRING_LENGTH (array)" bytes, but that is
not always the case, for languages that don't always use
zero-terminated strings, for instance hollerith strings in fortran.

--- gcc/expr.c  2018-08-17 05:32:57.332211963 +0200
+++ gcc/expr.c  2018-08-16 23:08:23.544940795 +0200
@@ -11372,6 +11372,9 @@ string_constant (tree arg, tree *ptr_off
   *ptr_offset = fold_convert (sizetype, offset);
   if (mem_size)
*mem_size = TYPE_SIZE_UNIT (TREE_TYPE (array));
+  else if (compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (array)),
+TREE_STRING_LENGTH (array)) < 0)
+   return NULL_TREE;
   return array;
 }


The range check in c_getstr was refined as well:

This I added, because vla arrays can be initialized with string constants,
especially since the 71625 patch was installed:
In this case we end up with mem_size that fails to be constant.


@@ -14606,25 +14603,17 @@ c_getstr (tree src, unsigned HOST_WIDE_I
offset = tree_to_uhwi (offset_node);
 }

+  if (!tree_fits_uhwi_p (mem_size))
+return NULL;
+
   /* STRING_LENGTH is the size of the string literal, including any
  embedded NULs.  STRING_SIZE is the size of the array the string
  literal is stored in.  */

Also the rest of the string length checks are refined, to return
actually zero-terminated single byte strings when strlen is not given,
and return something not necessarily zero-terminated which is
suitable for memxxx-functions otherwise.


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


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

	PR middle-end/86711
	PR middle-end/86714
	* expr.c (string_constant): Don't return truncated string literals.
	* fold-const.c (c_getstr): Fix function comment.  Remove unused third
	argument.  Fix range checks.
	* fold-const.c (c_getstr): Adjust protoype.

testsuite:
2018-08-17  Bernd Edlinger  

	PR middle-end/86711
	PR middle-end/86714
	* gcc.c-torture/execute/pr86711.c: New test.
	* gcc.c-torture/execute/pr86714.c: New test.

diff -Npur gcc/expr.c gcc/expr.c
--- gcc/expr.c	2018-08-17 05:32:57.332211963 +0200
+++ gcc/expr.c	2018-08-16 23:08:23.544940795 +0200
@@ -11372,6 +11372,9 @@ string_constant (tree arg, tree *ptr_off
   *ptr_offset = fold_convert (sizetype, offset);
   if (mem_size)
 	*mem_size = TYPE_SIZE_UNIT (TREE_TYPE (array));
+  else if (compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (array)),
+ TREE_STRING_LENGTH (array)) < 0)
+	return NULL_TREE;
   return array;
 }
 
@@ -11414,26 +11417,10 @@ string_constant (tree arg, tree *ptr_off
   if (!init || TREE_CODE (init) != STRING_CST)
 return NULL_TREE;
 
-  tree array_size = DECL_SIZE_UNIT (array);
-  if (!array_size || TREE_CODE (array_size) != INTEGER_CST)
-return NULL_TREE;
-
-  /* Avoid returning a string that doesn't fit in the array
- it is stored in, like
- const char a[4] = "abcde";
- but do handle those that fit even if they have excess
- initializers, such as in
- const char a[4] = "abc\000\000";
- The excess elements contribute to TREE_STRING_LENGTH()
- but not to strlen().  */
-  unsigned HOST_WIDE_INT charsize
-= tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (init;
-  unsigned HOST_WIDE_INT length = TREE_STRING_LENGTH (init);
-  length = string_length (TREE_STRING_POINTER (init), charsize,
-			  length / charsize);
   if (mem_size)
 *mem_size = TYPE_SIZE_UNIT (TREE_TYPE (init));
-  else if (compare_tree_int (array_size, length + 1) < 0)
+  else if (compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (init)),
+			 TREE_STRING_LENGTH (init)) < 0)
 return NULL_TREE;
 
   *ptr_offset = offset;
diff -Npur gcc/fold-const.c gcc/fold-const.c
--- gcc/fold-const.c	2018-07-16 08:49:39.0 +0200
+++ gcc/fold-const.c	2018-08-16 23:31:11.490869136 +0200
@@ -14577,23 +14577,20 @@ fold_build_pointer_plus_hwi_loc (locatio
 /* Return a pointer P to a NUL-terminated string representing the sequence
of constant characters referred to by SRC (or a subsequence of such
characters within it if SRC is a reference to a string plus some
-   constant offset).  If STRLEN is non-null, store stgrlen(P) in *STRLEN.
-   If STRSIZE is non-null, store in *STRSIZE the size of the array
-   the string is stored in; in that case, even though P points to a NUL
-   terminated string, SRC need not refer to one.  This can happen when
-   SRC refers to a constant character array initialized to all non-NUL
-   values, as in the C declaration: char a[4] = "1234";  */
+   constant offset).  If STRLEN is non-null, store the number of bytes
+   in the string constant including the terminating NUL 

Re: [PATCH 4/4] rs6000: Delete old add+cmp patterns

[Steven Bosscher]

On Thu, Aug 16, 2018 at 7:14 PM, Segher Boessenkool <> wrote:
> There are some patterns that recognise the parallel of an add and a
> compare, and split it back to the same two insns.  This apparently
> helped RIOS machines before RTL scheduling existed?  Either way, it
> isn't helpful anymore, and even hurts a tiny bit.  So, delete it.

It's been there since r251 (initial revision).

On the topic of archaeology: Perhaps the time also finally has come to
revisit this comment in rs6000.md:

13065; FIXME: This would probably be somewhat simpler if the Cygnus sibcall
13066; stuff was in GCC. <...>

:-)

Ciao!
Steven

Re: [PATCH] DWARF: add DW_AT_count to zero-length arrays

[Omar Sandoval]

On Thu, Aug 16, 2018 at 11:54:53PM -0700, Omar Sandoval wrote:
> On Thu, Aug 16, 2018 at 10:27:48PM -0700, Andrew Pinski wrote:
> > On Thu, Aug 16, 2018 at 9:29 PM Omar Sandoval  wrote:
> > >
> > > Hi,
> > >
> > > This fixes the issue that it is impossible to distinguish a zero-length 
> > > array
> > > type from a flexible array type given the DWARF produced by GCC (which I
> > > reported here [1]). We do so by adding a DW_AT_count attribute with a 
> > > value of
> > > zero only for zero-length arrays (this is what clang does in this case, 
> > > too).
> > >
> > > 1: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86985
> > >
> > > The reproducer from the PR now produces the expected output:
> > >
> > > $ ~/gcc-build/gcc/xgcc -B ~/gcc-build/gcc -g -c zero_length.c
> > > $ ~/gcc-build/gcc/xgcc -B ~/gcc-build/gcc -g -c flexible.c
> > > $ gdb -batch -ex 'ptype baz' zero_length.o
> > > type = struct {
> > > int foo;
> > > int bar[0];
> > > }
> > > $ gdb -batch -ex 'ptype baz' flexible.o
> > > type = struct {
> > > int foo;
> > > int bar[];
> > > }
> > >
> > > This was bootstrapped and tested on x86_64-pc-linux-gnu.
> > >
> > > I don't have commit rights (first time contributor), so if this change is 
> > > okay
> > > could it please be applied?

[snip]

Here's the patch with the is_c () helper instead.

2018-08-17  Omar Sandoval  

* dwarf2out.c (is_c): New.
(add_subscript_info): Add DW_AT_count of 0 for C zero-length arrays.

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 5a74131d332..189f9bb381f 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -3671,6 +3671,7 @@ static const char *get_AT_string (dw_die_ref, enum 
dwarf_attribute);
 static int get_AT_flag (dw_die_ref, enum dwarf_attribute);
 static unsigned get_AT_unsigned (dw_die_ref, enum dwarf_attribute);
 static inline dw_die_ref get_AT_ref (dw_die_ref, enum dwarf_attribute);
+static bool is_c (void);
 static bool is_cxx (void);
 static bool is_cxx (const_tree);
 static bool is_fortran (void);
@@ -5434,6 +5435,19 @@ get_AT_file (dw_die_ref die, enum dwarf_attribute 
attr_kind)
   return a ? AT_file (a) : NULL;
 }
 
+/* Return TRUE if the language is C.  */
+
+static inline bool
+is_c (void)
+{
+  unsigned int lang = get_AT_unsigned (comp_unit_die (), DW_AT_language);
+
+  return (lang == DW_LANG_C || lang == DW_LANG_C89 || lang == DW_LANG_C99
+ || lang == DW_LANG_C11 || lang == DW_LANG_ObjC);
+
+
+}
+
 /* Return TRUE if the language is C++.  */
 
 static inline bool
@@ -20918,12 +20932,24 @@ add_subscript_info (dw_die_ref type_die, tree type, 
bool collapse_p)
   dimension arr(N:*)
 Since the debugger is definitely going to need to know N
 to produce useful results, go ahead and output the lower
-bound solo, and hope the debugger can cope.  */
+bound solo, and hope the debugger can cope.
+
+For C and C++, if upper is NULL, this may be a zero-length array
+or a flexible array; we'd like to be able to distinguish between
+the two.  Set DW_AT_count to 0 for the former.  TYPE_SIZE is NULL
+for the latter.  */
 
  if (!get_AT (subrange_die, DW_AT_lower_bound))
add_bound_info (subrange_die, DW_AT_lower_bound, lower, NULL);
- if (upper && !get_AT (subrange_die, DW_AT_upper_bound))
-   add_bound_info (subrange_die, DW_AT_upper_bound, upper, NULL);
+ if (!get_AT (subrange_die, DW_AT_upper_bound)
+ && !get_AT (subrange_die, DW_AT_count))
+   {
+ if (upper)
+   add_bound_info (subrange_die, DW_AT_upper_bound, upper, NULL);
+ else if ((is_c () || is_cxx ()) && TYPE_SIZE (type))
+   add_bound_info (subrange_die, DW_AT_count,
+   build_int_cst (TREE_TYPE (lower), 0), NULL);
+   }
}
 
   /* Otherwise we have an array type with an unspecified length.  The

VRP: abstract out MIN/MAX/ABS wide int code

[Aldy Hernandez]

No change in functionality, just a straight up conversion.

OK for trunk?
gcc/

	* wide-int-range.cc (wide_int_range_abs): New.
	(wide_int_range_order_set): Rename from wide_int_range_min_max.
	* wide-int-range.h (wide_int_range_abs): New.
	(wide_int_range_min_max): New.
	* tree-vrp.c (extract_range_from_unary_expr): Rewrite ABS_EXPR
	case to call wide_int_range_abs.
	Rewrite MIN/MAX_EXPR to call wide_int_range_min_max.
	(extract_range_from_abs_expr): Delete.

diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index d553a254878..50e9eb0da07 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -1589,50 +1589,19 @@ extract_range_from_binary_expr_1 (value_range *vr,
   else if (code == MIN_EXPR
 	   || code == MAX_EXPR)
 {
-  if (vr0.type == VR_RANGE
-	  && !symbolic_range_p ())
-	{
-	  type = VR_RANGE;
-	  if (vr1.type == VR_RANGE
-	  && !symbolic_range_p ())
-	{
-	  /* For operations that make the resulting range directly
-		 proportional to the original ranges, apply the operation to
-		 the same end of each range.  */
-	  min = int_const_binop (code, vr0.min, vr1.min);
-	  max = int_const_binop (code, vr0.max, vr1.max);
-	}
-	  else if (code == MIN_EXPR)
-	{
-	  min = vrp_val_min (expr_type);
-	  max = vr0.max;
-	}
-	  else if (code == MAX_EXPR)
-	{
-	  min = vr0.min;
-	  max = vrp_val_max (expr_type);
-	}
-	}
-  else if (vr1.type == VR_RANGE
-	   && !symbolic_range_p ())
-	{
-	  type = VR_RANGE;
-	  if (code == MIN_EXPR)
-	{
-	  min = vrp_val_min (expr_type);
-	  max = vr1.max;
-	}
-	  else if (code == MAX_EXPR)
-	{
-	  min = vr1.min;
-	  max = vrp_val_max (expr_type);
-	}
-	}
+  wide_int wmin, wmax;
+  wide_int vr0_min, vr0_max;
+  wide_int vr1_min, vr1_max;
+  extract_range_into_wide_ints (, sign, prec, _min, _max);
+  extract_range_into_wide_ints (, sign, prec, _min, _max);
+  if (wide_int_range_min_max (wmin, wmax, code, sign, prec,
+  vr0_min, vr0_max, vr1_min, vr1_max))
+	set_value_range (vr, VR_RANGE,
+			 wide_int_to_tree (expr_type, wmin),
+			 wide_int_to_tree (expr_type, wmax), NULL);
   else
-	{
-	  set_value_range_to_varying (vr);
-	  return;
-	}
+	set_value_range_to_varying (vr);
+  return;
 }
   else if (code == MULT_EXPR)
 {
@@ -1919,85 +1888,6 @@ extract_range_from_binary_expr_1 (value_range *vr,
 set_value_range (vr, type, min, max, NULL);
 }
 
-/* Calculates the absolute value of a range and puts the result in VR.
-   VR0 is the input range.  TYPE is the type of the resulting
-   range.  */
-
-static void
-extract_range_from_abs_expr (value_range , tree type, value_range )
-{
-  /* Pass through vr0 in the easy cases.  */
-  if (TYPE_UNSIGNED (type)
-  || value_range_nonnegative_p ())
-{
-  copy_value_range (, );
-  return;
-}
-
-  /* For the remaining varying or symbolic ranges we can't do anything
- useful.  */
-  if (vr0.type == VR_VARYING
-  || symbolic_range_p ())
-{
-  set_value_range_to_varying ();
-  return;
-}
-
-  /* -TYPE_MIN_VALUE = TYPE_MIN_VALUE with flag_wrapv so we can't get a
- useful range.  */
-  if (!TYPE_OVERFLOW_UNDEFINED (type)
-  && ((vr0.type == VR_RANGE
-	   && vrp_val_is_min (vr0.min))
-	  || (vr0.type == VR_ANTI_RANGE
-	  && !vrp_val_is_min (vr0.min
-{
-  set_value_range_to_varying ();
-  return;
-}
-
-  /* ABS_EXPR may flip the range around, if the original range
- included negative values.  */
-  tree min, max;
-  if (!vrp_val_is_min (vr0.min))
-min = fold_unary_to_constant (ABS_EXPR, type, vr0.min);
-  else
-min = TYPE_MAX_VALUE (type);
-
-  if (!vrp_val_is_min (vr0.max))
-max = fold_unary_to_constant (ABS_EXPR, type, vr0.max);
-  else
-max = TYPE_MAX_VALUE (type);
-
-  int cmp = compare_values (min, max);
-  gcc_assert (vr0.type != VR_ANTI_RANGE);
-
-  /* If the range contains zero then we know that the minimum value in the
- range will be zero.  */
-  if (range_includes_zero_p (vr0.min, vr0.max) == 1)
-{
-  if (cmp == 1)
-	max = min;
-  min = build_int_cst (type, 0);
-}
-  else
-{
-  /* If the range was reversed, swap MIN and MAX.  */
-  if (cmp == 1)
-	std::swap (min, max);
-}
-
-  cmp = compare_values (min, max);
-  if (cmp == -2 || cmp == 1)
-{
-  /* If the new range has its limits swapped around (MIN > MAX),
-	 then the operation caused one of them to wrap around, mark
-	 the new range VARYING.  */
-  set_value_range_to_varying ();
-}
-  else
-set_value_range (, vr0.type, min, max, NULL);
-}
-
 /* Extract range information from a unary operation CODE based on
the range of its operand *VR0 with type OP0_TYPE with resulting type TYPE.
The resulting range is stored in *VR.  */
@@ -2007,6 +1897,8 @@ extract_range_from_unary_expr (value_range *vr,
 			   enum tree_code code, tree type,
 			   value_range *vr0_, tree op0_type)
 {
+  signop sign = 

Re: [PATCH] DWARF: add DW_AT_count to zero-length arrays

[Omar Sandoval]

On Thu, Aug 16, 2018 at 10:27:48PM -0700, Andrew Pinski wrote:
> On Thu, Aug 16, 2018 at 9:29 PM Omar Sandoval  wrote:
> >
> > Hi,
> >
> > This fixes the issue that it is impossible to distinguish a zero-length 
> > array
> > type from a flexible array type given the DWARF produced by GCC (which I
> > reported here [1]). We do so by adding a DW_AT_count attribute with a value 
> > of
> > zero only for zero-length arrays (this is what clang does in this case, 
> > too).
> >
> > 1: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86985
> >
> > The reproducer from the PR now produces the expected output:
> >
> > $ ~/gcc-build/gcc/xgcc -B ~/gcc-build/gcc -g -c zero_length.c
> > $ ~/gcc-build/gcc/xgcc -B ~/gcc-build/gcc -g -c flexible.c
> > $ gdb -batch -ex 'ptype baz' zero_length.o
> > type = struct {
> > int foo;
> > int bar[0];
> > }
> > $ gdb -batch -ex 'ptype baz' flexible.o
> > type = struct {
> > int foo;
> > int bar[];
> > }
> >
> > This was bootstrapped and tested on x86_64-pc-linux-gnu.
> >
> > I don't have commit rights (first time contributor), so if this change is 
> > okay
> > could it please be applied?
> 
> I don't think is really required.  Zero-sized arrays are a GCC
> extension which was introduced before flexible array types were part
> of C.  They should be interchangable in all places.  Can you give an
> example of where they are not?

See https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html. All of the
following cases are only valid for zero-length arrays and not flexible
arrays:

sizeof(int [0]);

struct foo {
int bar[0];
};

struct foo {
int bar[0];
int baz;
};

union foo {
int bar[0];
int baz;
};

int arr[2][0];

The following is only valid for flexible arrays and not zero-length
arrays:

struct {
int foo;
int bar[];
} baz = {1, {2, 3}};

> A comment about the patch below.
> 
> >
> > Thanks!
> >
> > 2018-08-16  Omar Sandoval  
> >
> > * dwarf2out.c (is_c_family): New.
> > (add_subscript_info): Add DW_AT_count of 0 for C zero-length arrays.
> >
> > diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> > index 5a74131d332..b638942c156 100644
> > --- a/gcc/dwarf2out.c
> > +++ b/gcc/dwarf2out.c
> > @@ -3671,6 +3671,7 @@ static const char *get_AT_string (dw_die_ref, enum 
> > dwarf_attribute);
> >  static int get_AT_flag (dw_die_ref, enum dwarf_attribute);
> >  static unsigned get_AT_unsigned (dw_die_ref, enum dwarf_attribute);
> >  static inline dw_die_ref get_AT_ref (dw_die_ref, enum dwarf_attribute);
> > +static bool is_c_family (void);
> >  static bool is_cxx (void);
> >  static bool is_cxx (const_tree);
> >  static bool is_fortran (void);
> > @@ -5434,6 +5435,21 @@ get_AT_file (dw_die_ref die, enum dwarf_attribute 
> > attr_kind)
> >return a ? AT_file (a) : NULL;
> >  }
> >
> > +/* Return TRUE if the language is C or C++.  */
> > +
> > +static inline bool
> > +is_c_family (void)
> > +{
> > +  unsigned int lang = get_AT_unsigned (comp_unit_die (), DW_AT_language);
> > +
> > +  return (lang == DW_LANG_C || lang == DW_LANG_C89 || lang == DW_LANG_C99
> > + || lang == DW_LANG_C11 || lang == DW_LANG_C_plus_plus
> > + || lang == DW_LANG_ObjC_plus_plus || lang == 
> > DW_LANG_C_plus_plus_11
> > + || lang == DW_LANG_C_plus_plus_14);
> > +
> > +
> > +}
> 
> I think you should just "is_cxx () || is_c ()" and factor out the
> C/Objective-C parts (C++ is already done).
> This is will make it easier to maintain so if c++17 or c++20 comes
> along, only one place needs to be changed.

Okay, will do.

Thanks for taking a look!

> > +
> >  /* Return TRUE if the language is C++.  */
> >
> >  static inline bool
> > @@ -20918,12 +20934,24 @@ add_subscript_info (dw_die_ref type_die, tree 
> > type, bool collapse_p)
> >dimension arr(N:*)
> >  Since the debugger is definitely going to need to know N
> >  to produce useful results, go ahead and output the lower
> > -bound solo, and hope the debugger can cope.  */
> > +bound solo, and hope the debugger can cope.
> > +
> > +For C and C++, if upper is NULL, this may be a zero-length 
> > array
> > +or a flexible array; we'd like to be able to distinguish 
> > between
> > +the two.  Set DW_AT_count to 0 for the former.  TYPE_SIZE is 
> > NULL
> > +for the latter.  */
> >
> >   if (!get_AT (subrange_die, DW_AT_lower_bound))
> > add_bound_info (subrange_die, DW_AT_lower_bound, lower, NULL);
> > - if (upper && !get_AT (subrange_die, DW_AT_upper_bound))
> > -   add_bound_info (subrange_die, DW_AT_upper_bound, upper, NULL);
> > + if (!get_AT (subrange_die, DW_AT_upper_bound)
> > + && !get_AT (subrange_die, DW_AT_count))
> > +   {
> > + if (upper)
> > +   add_bound_info (subrange_die, DW_AT_upper_bound, upper, 
> > NULL);
> > + else if (is_c_family () &&