[PATCH] Fix the damage done by my other patch from yesterday to strlenopt-49.c

[PATCH] Fix the damage done by my other patch from yesterday to strlenopt-49.c

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 563 bytes --]

Hi,

this is how I would like to handle the over length strings issue in the C FE.
If the string constant is exactly the right length and ends in one explicit
NUL character, shorten it by one character.

I thought Martin would be working on it,  but as this is a really simple fix,
I would dare to send it to gcc-patches anyway, hope you don't mind...

The patch is relative to the other patch here: https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01800.html


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


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-overlength-strings.diff --]
[-- Type: text/x-patch; name="patch-overlength-strings.diff", Size: 3713 bytes --]

gcc/c:
2018-07-30  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-typeck.c (digest_init): Fix overlength strings.

testsuite:
2018-07-30  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* gcc.dg/strlenopt-49.c: Adjust test expectations.

diff -pur gcc/c/c-typeck.c gcc/c/c-typeck.c
--- gcc/c/c-typeck.c	2018-06-20 18:35:15.000000000 +0200
+++ gcc/c/c-typeck.c	2018-07-30 12:17:34.175481372 +0200
@@ -7435,29 +7435,38 @@ digest_init (location_t init_loc, tree t
 		}
 	    }
 
-	  TREE_TYPE (inside_init) = type;
 	  if (TYPE_DOMAIN (type) != NULL_TREE
 	      && TYPE_SIZE (type) != NULL_TREE
 	      && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST)
 	    {
 	      unsigned HOST_WIDE_INT len = TREE_STRING_LENGTH (inside_init);
+	      unsigned unit = TYPE_PRECISION (typ1) / BITS_PER_UNIT;
 
 	      /* Subtract the size of a single (possibly wide) character
 		 because it's ok to ignore the terminating null char
 		 that is counted in the length of the constant.  */
-	      if (compare_tree_int (TYPE_SIZE_UNIT (type),
-				    (len - (TYPE_PRECISION (typ1)
-					    / BITS_PER_UNIT))) < 0)
+	      if (compare_tree_int (TYPE_SIZE_UNIT (type), len - unit) < 0)
 		pedwarn_init (init_loc, 0,
 			      ("initializer-string for array of chars "
 			       "is too long"));
-	      else if (warn_cxx_compat
-		       && compare_tree_int (TYPE_SIZE_UNIT (type), len) < 0)
-		warning_at (init_loc, OPT_Wc___compat,
-			    ("initializer-string for array chars "
-			     "is too long for C++"));
+	      else if (compare_tree_int (TYPE_SIZE_UNIT (type), len) < 0)
+		{
+		  if (warn_cxx_compat)
+		    warning_at (init_loc, OPT_Wc___compat,
+				("initializer-string for array chars "
+				 "is too long for C++"));
+		  if (len >= 2 * unit)
+		    {
+		      const char *p = TREE_STRING_POINTER (inside_init);
+
+		      len -= unit;
+		      if (memcmp (p + len - unit, "\0\0\0\0", unit) == 0)
+			inside_init = build_string (len, p);
+		    }
+		}
 	    }
 
+	  TREE_TYPE (inside_init) = type;
 	  return inside_init;
 	}
       else if (INTEGRAL_TYPE_P (typ1))
diff -pur gcc/testsuite/gcc.dg/strlenopt-49.c gcc/testsuite/gcc.dg/strlenopt-49.c
--- gcc/testsuite/gcc.dg/strlenopt-49.c	2018-07-30 13:02:34.735478726 +0200
+++ gcc/testsuite/gcc.dg/strlenopt-49.c	2018-07-30 13:08:21.074859303 +0200
@@ -11,9 +11,6 @@ const char a3[3] = "12\0";
 const char a8[8] = "1234567\0";
 const char a9[9] = "12345678\0";
 
-const char ax[9] = "12345678\0\0\0\0";   /* { dg-warning "initializer-string for array of chars is too long" } */
-const char ay[9] = "\00012345678\0\0\0\0";   /* { dg-warning "initializer-string for array of chars is too long" } */
-
 
 int len1 (void)
 {
@@ -27,27 +24,13 @@ int len (void)
   return len;
 }
 
-int lenx (void)
-{
-  size_t lenx = strlen (ax);
-  return lenx;
-}
-
-int leny (void)
-{
-  size_t leny = strlen (ay);
-  return leny;
-}
-
 int cmp88 (void)
 {
   int cmp88 = memcmp (a8, "1234567\0", sizeof a8);
   return cmp88;
 }
 
-/* { dg-final { scan-tree-dump-times "strlen" 0 "gimple" { xfail *-*-* } } }
-   { dg-final { scan-tree-dump-times "len0 = 0;" 1 "gimple" { xfail *-*-* } } }
-   { dg-final { scan-tree-dump-times "len = 18;" 1 "gimple" { xfail *-*-* } } }
-   { dg-final { scan-tree-dump-times "lenx = 8;" 1 "gimple" { xfail *-*-* } } }
-   { dg-final { scan-tree-dump-times "leny = 0;" 1 "gimple" { xfail *-*-* } } }
-   { dg-final { scan-tree-dump-times "cmp88 = 0;" 1 "gimple" { xfail *-*-* } } } */
+/* { dg-final { scan-tree-dump-times "strlen" 0 "gimple" } }
+   { dg-final { scan-tree-dump-times "len0 = 0;" 1 "gimple" } }
+   { dg-final { scan-tree-dump-times "len = 18;" 1 "gimple" } }
+   { dg-final { scan-tree-dump-times "cmp88 = 0;" 1 "gimple" } } */

Re: [PATCH] Fix the damage done by my other patch from yesterday to strlenopt-49.c

[Richard Biener]

On Mon, 30 Jul 2018, Bernd Edlinger wrote:

> Hi,
> 
> this is how I would like to handle the over length strings issue in the C FE.
> If the string constant is exactly the right length and ends in one explicit
> NUL character, shorten it by one character.
> 
> I thought Martin would be working on it,  but as this is a really simple fix,
> I would dare to send it to gcc-patches anyway, hope you don't mind...
> 
> The patch is relative to the other patch here: https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01800.html
> 
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?

I'll leave this to FE maintainers but can I ask you to verify the
(other) FEs do not leak this kind of invalid initializers to the
middle-end?  I suggest to put this verification in
output_constructor which otherwise happily truncates initializers
with excess size.  There's also gimplification which might elide
a = { "abcd", "cdse" }; to  a.x = "abcd"; a.y = "cdse"; but
hopefully there the GIMPLE verifier (verify_gimple_assign_single)
verifies this - well, it only dispatches to useless_type_conversion_p 
(lhs_type, rhs1_type) for this case, but non-flexarrays should be
handled fine there.

Richard.

Re: [PATCH] Fix the damage done by my other patch from yesterday to strlenopt-49.c

[Bernd Edlinger]

On 07/30/18 15:03, Richard Biener wrote:
> On Mon, 30 Jul 2018, Bernd Edlinger wrote:
> 
>> Hi,
>>
>> this is how I would like to handle the over length strings issue in the C FE.
>> If the string constant is exactly the right length and ends in one explicit
>> NUL character, shorten it by one character.
>>
>> I thought Martin would be working on it,  but as this is a really simple fix,
>> I would dare to send it to gcc-patches anyway, hope you don't mind...
>>
>> The patch is relative to the other patch here: https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01800.html
>>
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
> 
> I'll leave this to FE maintainers but can I ask you to verify the
> (other) FEs do not leak this kind of invalid initializers to the
> middle-end?  I suggest to put this verification in
> output_constructor which otherwise happily truncates initializers
> with excess size.  There's also gimplification which might elide
> a = { "abcd", "cdse" }; to  a.x = "abcd"; a.y = "cdse"; but
> hopefully there the GIMPLE verifier (verify_gimple_assign_single)
> verifies this - well, it only dispatches to useless_type_conversion_p
> (lhs_type, rhs1_type) for this case, but non-flexarrays should be
> handled fine there.
> 
> Richard.
> 

In the moment I would already be happy if all STRING_CSTs would
be zero terminated.

However Go does not create zero-terminated STRING_CSTs, @Ian sorry,
could you look at changing this to include the terminating NUL char?

Index: gcc/go/go-gcc.cc
===================================================================
--- gcc/go/go-gcc.cc	(revision 263068)
+++ gcc/go/go-gcc.cc	(working copy)
@@ -1394,7 +1394,7 @@ Gcc_backend::string_constant_expression(const std:
  					      TYPE_QUAL_CONST);
    tree string_type = build_array_type(const_char_type, index_type);
    TYPE_STRING_FLAG(string_type) = 1;
-  tree string_val = build_string(val.length(), val.data());
+  tree string_val = build_string(val.length() + 1, val.data());
    TREE_TYPE(string_val) = string_type;
  
    return this->make_expression(string_val);


A am pretty sure that the C++ FE allows overlength initializers
with -permissive.  They should be hedged in string_constant IMHO,
however with the patch I am still holding back on Jeff's request
I ran over a string constant in tree-ssa-strlen.c (get_min_string_length)
that had a terminating NUL char but the index range type did not
include the string terminator.  One just needs to be careful here.

A quick survey shows that Fortran creates C strings with range
1..n, which puts the pr86532 address computation again in question.
Remember, you asked for array_ref_element_size but not for
array_ref_low_bound, and Jeff acked the patch in this state.



Thanks
Bernd.

Re: [PATCH] Fix the damage done by my other patch from yesterday to strlenopt-49.c

[Joseph Myers]

On Mon, 30 Jul 2018, Bernd Edlinger wrote:

> In the moment I would already be happy if all STRING_CSTs would
> be zero terminated.

generic.texi says they need not be.  Making the STRING_CST contain only 
the bytes of the initializer and not the trailing NUL in the C case where 
the trailing NUL does not fit in the object initialized would of course 
mean you get non-NUL-terminated STRING_CSTs for valid C code as well.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fix the damage done by my other patch from yesterday to strlenopt-49.c

[Jakub Jelinek]

On Mon, Jul 30, 2018 at 03:52:39PM +0000, Joseph Myers wrote:
> On Mon, 30 Jul 2018, Bernd Edlinger wrote:
> 
> > In the moment I would already be happy if all STRING_CSTs would
> > be zero terminated.
> 
> generic.texi says they need not be.  Making the STRING_CST contain only 
> the bytes of the initializer and not the trailing NUL in the C case where 
> the trailing NUL does not fit in the object initialized would of course 
> mean you get non-NUL-terminated STRING_CSTs for valid C code as well.

One thing is whether TREE_STRING_LENGTH includes the trailing NUL byte,
that doesn't need to be the case e.g. for the shortened initializers.
The other thing is whether we as a convenience for the compiler's internals
want to waste some memory for the NUL termination; I think we could avoid
some bugs that way.

	Jakub

Re: [PATCH] Fix the damage done by my other patch from yesterday to strlenopt-49.c

[Joseph Myers]

On Mon, 30 Jul 2018, Jakub Jelinek wrote:

> On Mon, Jul 30, 2018 at 03:52:39PM +0000, Joseph Myers wrote:
> > On Mon, 30 Jul 2018, Bernd Edlinger wrote:
> > 
> > > In the moment I would already be happy if all STRING_CSTs would
> > > be zero terminated.
> > 
> > generic.texi says they need not be.  Making the STRING_CST contain only 
> > the bytes of the initializer and not the trailing NUL in the C case where 
> > the trailing NUL does not fit in the object initialized would of course 
> > mean you get non-NUL-terminated STRING_CSTs for valid C code as well.
> 
> One thing is whether TREE_STRING_LENGTH includes the trailing NUL byte,
> that doesn't need to be the case e.g. for the shortened initializers.
> The other thing is whether we as a convenience for the compiler's internals
> want to waste some memory for the NUL termination; I think we could avoid
> some bugs that way.

TREE_STRING_LENGTH includes the NUL if it is logically part of the object, 
but should not if the NUL is purely an implementation convenience.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fix the damage done by my other patch from yesterday to strlenopt-49.c

[Bernd Edlinger]

On 07/30/18 18:01, Joseph Myers wrote:
> On Mon, 30 Jul 2018, Jakub Jelinek wrote:
> 
>> On Mon, Jul 30, 2018 at 03:52:39PM +0000, Joseph Myers wrote:
>>> On Mon, 30 Jul 2018, Bernd Edlinger wrote:
>>>
>>>> In the moment I would already be happy if all STRING_CSTs would
>>>> be zero terminated.
>>>
>>> generic.texi says they need not be.  Making the STRING_CST contain only
>>> the bytes of the initializer and not the trailing NUL in the C case where
>>> the trailing NUL does not fit in the object initialized would of course
>>> mean you get non-NUL-terminated STRING_CSTs for valid C code as well.
>>
>> One thing is whether TREE_STRING_LENGTH includes the trailing NUL byte,
>> that doesn't need to be the case e.g. for the shortened initializers.
>> The other thing is whether we as a convenience for the compiler's internals
>> want to waste some memory for the NUL termination; I think we could avoid
>> some bugs that way.
> 
> TREE_STRING_LENGTH includes the NUL if it is logically part of the object,
> but should not if the NUL is purely an implementation convenience.
> 

To complicate things a bit more STRING_CST that are created by the Ada FE
for the purpose of ASM constraints, do not contain a NUL character.
That is in TREE_STRING_LENGTH, there is of course always another NUL char
after the payload.  Likewise for C __attribute__ values.


Thanks
Bernd.

Re: [PATCH] Fix the damage done by my other patch from yesterday to strlenopt-49.c

[Jakub Jelinek]

On Mon, Jul 30, 2018 at 04:28:50PM +0000, Bernd Edlinger wrote:
> >>> generic.texi says they need not be.  Making the STRING_CST contain only
> >>> the bytes of the initializer and not the trailing NUL in the C case where
> >>> the trailing NUL does not fit in the object initialized would of course
> >>> mean you get non-NUL-terminated STRING_CSTs for valid C code as well.
> >>
> >> One thing is whether TREE_STRING_LENGTH includes the trailing NUL byte,
> >> that doesn't need to be the case e.g. for the shortened initializers.
> >> The other thing is whether we as a convenience for the compiler's internals
> >> want to waste some memory for the NUL termination; I think we could avoid
> >> some bugs that way.
> > 
> > TREE_STRING_LENGTH includes the NUL if it is logically part of the object,
> > but should not if the NUL is purely an implementation convenience.
> > 
> 
> To complicate things a bit more STRING_CST that are created by the Ada FE
> for the purpose of ASM constraints, do not contain a NUL character.
> That is in TREE_STRING_LENGTH, there is of course always another NUL char
> after the payload.  Likewise for C __attribute__ values.

If there is a NUL at TREE_STRING_POINTER (x) + TREE_STRING_LENGTH (x), that
is ok.

	Jakub

Re: [PATCH] Fix the damage done by my other patch from yesterday to strlenopt-49.c

[Bernd Edlinger]

On 07/30/18 17:57, Jakub Jelinek wrote:
> On Mon, Jul 30, 2018 at 03:52:39PM +0000, Joseph Myers wrote:
>> On Mon, 30 Jul 2018, Bernd Edlinger wrote:
>>
>>> In the moment I would already be happy if all STRING_CSTs would
>>> be zero terminated.
>>
>> generic.texi says they need not be.  Making the STRING_CST contain only
>> the bytes of the initializer and not the trailing NUL in the C case where
>> the trailing NUL does not fit in the object initialized would of course
>> mean you get non-NUL-terminated STRING_CSTs for valid C code as well.
> 
> One thing is whether TREE_STRING_LENGTH includes the trailing NUL byte,
> that doesn't need to be the case e.g. for the shortened initializers.
> The other thing is whether we as a convenience for the compiler's internals
> want to waste some memory for the NUL termination; I think we could avoid
> some bugs that way.
> 

Yes, exactly, currently the middle-end tries determine if a STRING_CST
is nul terminated, but that is broken for wide character, for instance
c_getstr:

   else if (string[string_length - 1] != '\0')
     {
       /* Support only properly NUL-terminated strings but handle
          consecutive strings within the same array, such as the six
          substrings in "1\0002\0003".  */
       return NULL;
     }

It would be much better if any string constant could be zero terminated.

When always zero-terminated STRING_CST the check for a non-zero terminated
value is much more easy:

compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (init)), TREE_STRING_LENGTH (init))



Bernd.

Re: [PATCH] Fix the damage done by my other patch from yesterday to strlenopt-49.c

[Richard Biener]

On July 30, 2018 4:41:19 PM GMT+02:00, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
>
>
>On 07/30/18 15:03, Richard Biener wrote:
>> On Mon, 30 Jul 2018, Bernd Edlinger wrote:
>> 
>>> Hi,
>>>
>>> this is how I would like to handle the over length strings issue in
>the C FE.
>>> If the string constant is exactly the right length and ends in one
>explicit
>>> NUL character, shorten it by one character.
>>>
>>> I thought Martin would be working on it,  but as this is a really
>simple fix,
>>> I would dare to send it to gcc-patches anyway, hope you don't
>mind...
>>>
>>> The patch is relative to the other patch here:
>https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01800.html
>>>
>>>
>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>> Is it OK for trunk?
>> 
>> I'll leave this to FE maintainers but can I ask you to verify the
>> (other) FEs do not leak this kind of invalid initializers to the
>> middle-end?  I suggest to put this verification in
>> output_constructor which otherwise happily truncates initializers
>> with excess size.  There's also gimplification which might elide
>> a = { "abcd", "cdse" }; to  a.x = "abcd"; a.y = "cdse"; but
>> hopefully there the GIMPLE verifier (verify_gimple_assign_single)
>> verifies this - well, it only dispatches to useless_type_conversion_p
>> (lhs_type, rhs1_type) for this case, but non-flexarrays should be
>> handled fine there.
>> 
>> Richard.
>> 
>
>In the moment I would already be happy if all STRING_CSTs would
>be zero terminated.
>
>However Go does not create zero-terminated STRING_CSTs, @Ian sorry,
>could you look at changing this to include the terminating NUL char?
>
>Index: gcc/go/go-gcc.cc
>===================================================================
>--- gcc/go/go-gcc.cc	(revision 263068)
>+++ gcc/go/go-gcc.cc	(working copy)
>@@ -1394,7 +1394,7 @@ Gcc_backend::string_constant_expression(const
>std:
>  					      TYPE_QUAL_CONST);
>    tree string_type = build_array_type(const_char_type, index_type);
>    TYPE_STRING_FLAG(string_type) = 1;
>-  tree string_val = build_string(val.length(), val.data());
>+  tree string_val = build_string(val.length() + 1, val.data());
>    TREE_TYPE(string_val) = string_type;
>  
>    return this->make_expression(string_val);
>
>
>A am pretty sure that the C++ FE allows overlength initializers
>with -permissive.  They should be hedged in string_constant IMHO,
>however with the patch I am still holding back on Jeff's request
>I ran over a string constant in tree-ssa-strlen.c
>(get_min_string_length)
>that had a terminating NUL char but the index range type did not
>include the string terminator.  One just needs to be careful here.
>
>A quick survey shows that Fortran creates C strings with range
>1..n, which puts the pr86532 address computation again in question.
>Remember, you asked for array_ref_element_size but not for
>array_ref_low_bound, and Jeff acked the patch in this state.

The (earlier) changes in this area are unfortunately in a quite messy state. It would be nice to roll back completely and attack this in smaller and more obvious issues. 

Richard. 

>
>
>Thanks
>Bernd.

Re: [PATCH] Fix the damage done by my other patch from yesterday to strlenopt-49.c

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1550 bytes --]

On 07/30/18 15:03, Richard Biener wrote:
> On Mon, 30 Jul 2018, Bernd Edlinger wrote:
> 
>> Hi,
>>
>> this is how I would like to handle the over length strings issue in the C FE.
>> If the string constant is exactly the right length and ends in one explicit
>> NUL character, shorten it by one character.
>>
>> I thought Martin would be working on it,  but as this is a really simple fix,
>> I would dare to send it to gcc-patches anyway, hope you don't mind...
>>
>> The patch is relative to the other patch here: https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01800.html
>>
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
> 
> I'll leave this to FE maintainers but can I ask you to verify the
> (other) FEs do not leak this kind of invalid initializers to the
> middle-end?  I suggest to put this verification in
> output_constructor which otherwise happily truncates initializers
> with excess size.  There's also gimplification which might elide
> a = { "abcd", "cdse" }; to  a.x = "abcd"; a.y = "cdse"; but
> hopefully there the GIMPLE verifier (verify_gimple_assign_single)
> verifies this - well, it only dispatches to useless_type_conversion_p
> (lhs_type, rhs1_type) for this case, but non-flexarrays should be
> handled fine there.
> 

Okay, this is what I am currently playing with.
There is still a major fault in the fortran FE, but I believe sanitizing
the middle-end is worth it....

IMHO sanitizing should have priority over new optimizations :(


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-varasm.diff --]
[-- Type: text/x-patch; name="patch-varasm.diff", Size: 1462 bytes --]

Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c	(Revision 263072)
+++ gcc/varasm.c	(Arbeitskopie)
@@ -4774,6 +4774,29 @@ initializer_constant_valid_for_bitfield_p (tree va
   return false;
 }
 
+/* Check if a STRING_CST fits into the field.
+   Tolerate only the case when the NUL termination
+   does not fit into the field.   */
+
+bool
+check_string_literal (tree string, unsigned HOST_WIDE_INT size)
+{
+  tree eltype = TREE_TYPE (TREE_TYPE (string));
+  unsigned HOST_WIDE_INT elts = tree_to_uhwi (TYPE_SIZE_UNIT (eltype));
+  const char *p = TREE_STRING_POINTER (string);
+  int len = TREE_STRING_LENGTH (string);
+
+  if (elts != 1 && elts != 2 && elts != 4)
+    return false;
+  if (len <= 0 || len % elts != 0)
+    return false;
+  if ((unsigned)len != size && (unsigned)len != size + elts)
+    return false;
+  if (memcmp (p + len - elts, "\0\0\0\0", elts) != 0)
+    return false;
+  return true;
+}
+
 /* output_constructor outer state of relevance in recursive calls, typically
    for nested aggregate bitfields.  */
 
@@ -4942,6 +4965,7 @@ output_constant (tree exp, unsigned HOST_WIDE_INT
 	case STRING_CST:
 	  thissize
 	    = MIN ((unsigned HOST_WIDE_INT)TREE_STRING_LENGTH (exp), size);
+	  gcc_checking_assert (check_string_literal (exp, thissize));
 	  assemble_string (TREE_STRING_POINTER (exp), thissize);
 	  break;
 	case VECTOR_CST:

Re: [PATCH] Fix the damage done by my other patch from yesterday to strlenopt-49.c

[Martin Sebor]

On 07/30/2018 05:51 AM, Bernd Edlinger wrote:
> Hi,
>
> this is how I would like to handle the over length strings issue in the C FE.
> If the string constant is exactly the right length and ends in one explicit
> NUL character, shorten it by one character.
>
> I thought Martin would be working on it,  but as this is a really simple fix,
> I would dare to send it to gcc-patches anyway, hope you don't mind...

And as I said repeatedly, I am working on it along with a number
of other things in this very area.  There are a number of issues
to solve:

1) issue a warning for the non-nul terminated strings (bug 86552:
    raised in response to your comments, initial patch posted,
    more work in progress)
2) avoid creating overlong string literals (bug 86688: raised
    partly also in response to your earlier comments)
3) handle braced-initializers (as in char a[] = { 1, 2, 0 }; )
    I'm testing a patch for that.

I am actively working on all three of these so please, for the fourth
time, let me finish my work.  Submitting patches that try to handle
any of these issues in a slightly or substantially different way at
the same time isn't helpful.  On the contrary, it's very disruptive.
This has nothing to do with ownership and everything with
coordination and an apparent divergence of opinions.

There are over 10,000 open bugs in Bugzilla.  Working on any of
them that's not assigned to anyone would be helpful.  This is
not.

Martin

Re: [PATCH] Fix the damage done by my other patch from yesterday to strlenopt-49.c

[Joseph Myers]

On Mon, 30 Jul 2018, Bernd Edlinger wrote:

> Hi,
> 
> this is how I would like to handle the over length strings issue in the C FE.
> If the string constant is exactly the right length and ends in one explicit
> NUL character, shorten it by one character.

I don't think shortening should be limited to that case.  I think the case 
where the constant is longer than that (and so gets an unconditional 
pedwarn) should also have it shortened - any constant that doesn't fit in 
the object being initialized should be shortened to fit, whether diagnosed 
or not, we should define GENERIC / GIMPLE to disallow too-large string 
constants in initializers, and should add an assertion somewhere in the 
middle-end that no too-large string constants reach it.

-- 
Joseph S. Myers
joseph@codesourcery.com

[PATCH] Handle overlength strings in the C FE

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

On 07/30/18 17:49, Joseph Myers wrote:
> On Mon, 30 Jul 2018, Bernd Edlinger wrote:
> 
>> Hi,
>>
>> this is how I would like to handle the over length strings issue in the C FE.
>> If the string constant is exactly the right length and ends in one explicit
>> NUL character, shorten it by one character.
> 
> I don't think shortening should be limited to that case.  I think the case
> where the constant is longer than that (and so gets an unconditional
> pedwarn) should also have it shortened - any constant that doesn't fit in
> the object being initialized should be shortened to fit, whether diagnosed
> or not, we should define GENERIC / GIMPLE to disallow too-large string
> constants in initializers, and should add an assertion somewhere in the
> middle-end that no too-large string constants reach it.
> 

Okay, there is an update following your suggestion.

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


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-c-fe.diff --]
[-- Type: text/x-patch; name="patch-c-fe.diff", Size: 2525 bytes --]

2018-08-01  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-typeck.c (digest_init): Fix overlength strings.

diff -pur gcc/c/c-typeck.c gcc/c/c-typeck.c
--- gcc/c/c-typeck.c	2018-06-20 18:35:15.000000000 +0200
+++ gcc/c/c-typeck.c	2018-07-31 18:49:50.757586625 +0200
@@ -7435,29 +7435,52 @@ digest_init (location_t init_loc, tree t
 		}
 	    }
 
-	  TREE_TYPE (inside_init) = type;
 	  if (TYPE_DOMAIN (type) != NULL_TREE
 	      && TYPE_SIZE (type) != NULL_TREE
 	      && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST)
 	    {
 	      unsigned HOST_WIDE_INT len = TREE_STRING_LENGTH (inside_init);
+	      unsigned unit = TYPE_PRECISION (typ1) / BITS_PER_UNIT;
 
 	      /* Subtract the size of a single (possibly wide) character
 		 because it's ok to ignore the terminating null char
 		 that is counted in the length of the constant.  */
-	      if (compare_tree_int (TYPE_SIZE_UNIT (type),
-				    (len - (TYPE_PRECISION (typ1)
-					    / BITS_PER_UNIT))) < 0)
-		pedwarn_init (init_loc, 0,
-			      ("initializer-string for array of chars "
-			       "is too long"));
-	      else if (warn_cxx_compat
-		       && compare_tree_int (TYPE_SIZE_UNIT (type), len) < 0)
-		warning_at (init_loc, OPT_Wc___compat,
-			    ("initializer-string for array chars "
-			     "is too long for C++"));
+	      if (compare_tree_int (TYPE_SIZE_UNIT (type), len - unit) < 0)
+		{
+		  pedwarn_init (init_loc, 0,
+				("initializer-string for array of chars "
+				 "is too long"));
+		  if (tree_fits_uhwi_p (TYPE_SIZE_UNIT (type)))
+		    {
+		      unsigned HOST_WIDE_INT size
+			= tree_to_uhwi (TYPE_SIZE_UNIT (type));
+		      const char *p = TREE_STRING_POINTER (inside_init);
+		      char *q = (char *)xmalloc (size + unit);
+
+		      memcpy (q, p, size);
+		      memset (q + size, 0, unit);
+		      inside_init = build_string (size + unit, q);
+		      free (q);
+		    }
+		}
+	      else if (compare_tree_int (TYPE_SIZE_UNIT (type), len) < 0)
+		{
+		  if (warn_cxx_compat)
+		    warning_at (init_loc, OPT_Wc___compat,
+				("initializer-string for array chars "
+				 "is too long for C++"));
+		  if (len >= 2 * unit)
+		    {
+		      const char *p = TREE_STRING_POINTER (inside_init);
+
+		      len -= unit;
+		      if (memcmp (p + len - unit, "\0\0\0\0", unit) == 0)
+			inside_init = build_string (len, p);
+		    }
+		}
 	    }
 
+	  TREE_TYPE (inside_init) = type;
 	  return inside_init;
 	}
       else if (INTEGRAL_TYPE_P (typ1))

Re: [PATCH] Handle overlength strings in the C FE

[Joseph Myers]

On Wed, 1 Aug 2018, Bernd Edlinger wrote:

> On 07/30/18 17:49, Joseph Myers wrote:
> > On Mon, 30 Jul 2018, Bernd Edlinger wrote:
> > 
> >> Hi,
> >>
> >> this is how I would like to handle the over length strings issue in the C FE.
> >> If the string constant is exactly the right length and ends in one explicit
> >> NUL character, shorten it by one character.
> > 
> > I don't think shortening should be limited to that case.  I think the case
> > where the constant is longer than that (and so gets an unconditional
> > pedwarn) should also have it shortened - any constant that doesn't fit in
> > the object being initialized should be shortened to fit, whether diagnosed
> > or not, we should define GENERIC / GIMPLE to disallow too-large string
> > constants in initializers, and should add an assertion somewhere in the
> > middle-end that no too-large string constants reach it.
> > 
> 
> Okay, there is an update following your suggestion.

It seems odd to me to have two separate bits of code dealing with reducing 
the length, rather than something like

if (too long)
  {
    /* Decide whether to do a pedwarn_init, or a warn_cxx_compat warning,
       or neither.  */
    /* Shorten string, in either case.  */
  }

The memcmp with "\0\0\0\0" is introducing a hidden assumption that any 
sort of character in strings is never more than four bytes.  It also seems 
unnecessary, in that ultimately the over-long string should be shortened 
regardless of whether what's being removed is a zero character or not.

It should not be possible to be over-long and fail tree_fits_uhwi_p 
(TYPE_SIZE_UNIT (type)), simply because STRING_CST lengths are stored in 
host int (even if, ideally, they'd use some other type to allow for 
STRING_CSTs over 2GB in size).  (And I don't think GCC can represent 
target type sizes that don't fit in unsigned HOST_WIDE_INT anyway; the 
only way for a target type size in bytes to fail to be representable in 
unsigned HOST_WIDE_INT should be if the size is not constant.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Handle overlength strings in the C FE

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 2289 bytes --]

On 08/01/18 18:04, Joseph Myers wrote:
> On Wed, 1 Aug 2018, Bernd Edlinger wrote:
> 
>> On 07/30/18 17:49, Joseph Myers wrote:
>>> On Mon, 30 Jul 2018, Bernd Edlinger wrote:
>>>
>>>> Hi,
>>>>
>>>> this is how I would like to handle the over length strings issue in the C FE.
>>>> If the string constant is exactly the right length and ends in one explicit
>>>> NUL character, shorten it by one character.
>>>
>>> I don't think shortening should be limited to that case.  I think the case
>>> where the constant is longer than that (and so gets an unconditional
>>> pedwarn) should also have it shortened - any constant that doesn't fit in
>>> the object being initialized should be shortened to fit, whether diagnosed
>>> or not, we should define GENERIC / GIMPLE to disallow too-large string
>>> constants in initializers, and should add an assertion somewhere in the
>>> middle-end that no too-large string constants reach it.
>>>
>>
>> Okay, there is an update following your suggestion.
> 
> It seems odd to me to have two separate bits of code dealing with reducing
> the length, rather than something like
> 
> if (too long)
>    {
>      /* Decide whether to do a pedwarn_init, or a warn_cxx_compat warning,
>         or neither.  */
>      /* Shorten string, in either case.  */
>    }
> 
> The memcmp with "\0\0\0\0" is introducing a hidden assumption that any
> sort of character in strings is never more than four bytes.  It also seems
> unnecessary, in that ultimately the over-long string should be shortened
> regardless of whether what's being removed is a zero character or not.
> > It should not be possible to be over-long and fail tree_fits_uhwi_p
> (TYPE_SIZE_UNIT (type)), simply because STRING_CST lengths are stored in
> host int (even if, ideally, they'd use some other type to allow for
> STRING_CSTs over 2GB in size).  (And I don't think GCC can represent
> target type sizes that don't fit in unsigned HOST_WIDE_INT anyway; the
> only way for a target type size in bytes to fail to be representable in
> unsigned HOST_WIDE_INT should be if the size is not constant.)
> 

Agreed.
A new simplified version of the patch is attached.

Bootstrapped and reg-tested as usual.
Is it OK for trunk?


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-c-fe.diff --]
[-- Type: text/x-patch; name="patch-c-fe.diff", Size: 1905 bytes --]

2018-08-01  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-typeck.c (digest_init): Shorten overlength strings.

diff -pur gcc/c/c-typeck.c gcc/c/c-typeck.c
--- gcc/c/c-typeck.c	2018-06-20 18:35:15.000000000 +0200
+++ gcc/c/c-typeck.c	2018-07-31 18:49:50.757586625 +0200
@@ -7435,19 +7435,17 @@ digest_init (location_t init_loc, tree type, tree
 		}
 	    }
 
-	  TREE_TYPE (inside_init) = type;
 	  if (TYPE_DOMAIN (type) != NULL_TREE
 	      && TYPE_SIZE (type) != NULL_TREE
 	      && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST)
 	    {
 	      unsigned HOST_WIDE_INT len = TREE_STRING_LENGTH (inside_init);
+	      unsigned unit = TYPE_PRECISION (typ1) / BITS_PER_UNIT;
 
 	      /* Subtract the size of a single (possibly wide) character
 		 because it's ok to ignore the terminating null char
 		 that is counted in the length of the constant.  */
-	      if (compare_tree_int (TYPE_SIZE_UNIT (type),
-				    (len - (TYPE_PRECISION (typ1)
-					    / BITS_PER_UNIT))) < 0)
+	      if (compare_tree_int (TYPE_SIZE_UNIT (type), len - unit) < 0)
 		pedwarn_init (init_loc, 0,
 			      ("initializer-string for array of chars "
 			       "is too long"));
@@ -7456,8 +7454,21 @@ digest_init (location_t init_loc, tree type, tree
 		warning_at (init_loc, OPT_Wc___compat,
 			    ("initializer-string for array chars "
 			     "is too long for C++"));
+	      if (compare_tree_int (TYPE_SIZE_UNIT (type), len) < 0)
+		{
+		  unsigned HOST_WIDE_INT size
+		    = tree_to_uhwi (TYPE_SIZE_UNIT (type));
+		  const char *p = TREE_STRING_POINTER (inside_init);
+		  char *q = (char *)xmalloc (size + unit);
+
+		  memcpy (q, p, size);
+		  memset (q + size, 0, unit);
+		  inside_init = build_string (size + unit, q);
+		  free (q);
+		}
 	    }
 
+	  TREE_TYPE (inside_init) = type;
 	  return inside_init;
 	}
       else if (INTEGRAL_TYPE_P (typ1))

Re: [PATCH] Handle overlength strings in the C FE

[Marek Polacek]

On Wed, Aug 01, 2018 at 08:06:53PM +0000, Bernd Edlinger wrote:
> On 08/01/18 18:04, Joseph Myers wrote:
> > On Wed, 1 Aug 2018, Bernd Edlinger wrote:
> > 
> >> On 07/30/18 17:49, Joseph Myers wrote:
> >>> On Mon, 30 Jul 2018, Bernd Edlinger wrote:
> >>>
> >>>> Hi,
> >>>>
> >>>> this is how I would like to handle the over length strings issue in the C FE.
> >>>> If the string constant is exactly the right length and ends in one explicit
> >>>> NUL character, shorten it by one character.
> >>>
> >>> I don't think shortening should be limited to that case.  I think the case
> >>> where the constant is longer than that (and so gets an unconditional
> >>> pedwarn) should also have it shortened - any constant that doesn't fit in
> >>> the object being initialized should be shortened to fit, whether diagnosed
> >>> or not, we should define GENERIC / GIMPLE to disallow too-large string
> >>> constants in initializers, and should add an assertion somewhere in the
> >>> middle-end that no too-large string constants reach it.
> >>>
> >>
> >> Okay, there is an update following your suggestion.
> > 
> > It seems odd to me to have two separate bits of code dealing with reducing
> > the length, rather than something like
> > 
> > if (too long)
> >    {
> >      /* Decide whether to do a pedwarn_init, or a warn_cxx_compat warning,
> >         or neither.  */
> >      /* Shorten string, in either case.  */
> >    }
> > 
> > The memcmp with "\0\0\0\0" is introducing a hidden assumption that any
> > sort of character in strings is never more than four bytes.  It also seems
> > unnecessary, in that ultimately the over-long string should be shortened
> > regardless of whether what's being removed is a zero character or not.
> > > It should not be possible to be over-long and fail tree_fits_uhwi_p
> > (TYPE_SIZE_UNIT (type)), simply because STRING_CST lengths are stored in
> > host int (even if, ideally, they'd use some other type to allow for
> > STRING_CSTs over 2GB in size).  (And I don't think GCC can represent
> > target type sizes that don't fit in unsigned HOST_WIDE_INT anyway; the
> > only way for a target type size in bytes to fail to be representable in
> > unsigned HOST_WIDE_INT should be if the size is not constant.)
> > 
> 
> Agreed.
> A new simplified version of the patch is attached.
> 
> Bootstrapped and reg-tested as usual.
> Is it OK for trunk?
> 
> 
> Thanks
> Bernd.

> 2018-08-01  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* c-typeck.c (digest_init): Shorten overlength strings.
> 
> diff -pur gcc/c/c-typeck.c gcc/c/c-typeck.c
> --- gcc/c/c-typeck.c	2018-06-20 18:35:15.000000000 +0200
> +++ gcc/c/c-typeck.c	2018-07-31 18:49:50.757586625 +0200
> @@ -7435,19 +7435,17 @@ digest_init (location_t init_loc, tree type, tree
>  		}
>  	    }
>  
> -	  TREE_TYPE (inside_init) = type;
>  	  if (TYPE_DOMAIN (type) != NULL_TREE
>  	      && TYPE_SIZE (type) != NULL_TREE
>  	      && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST)
>  	    {
>  	      unsigned HOST_WIDE_INT len = TREE_STRING_LENGTH (inside_init);
> +	      unsigned unit = TYPE_PRECISION (typ1) / BITS_PER_UNIT;
>  
>  	      /* Subtract the size of a single (possibly wide) character
>  		 because it's ok to ignore the terminating null char
>  		 that is counted in the length of the constant.  */
> -	      if (compare_tree_int (TYPE_SIZE_UNIT (type),
> -				    (len - (TYPE_PRECISION (typ1)
> -					    / BITS_PER_UNIT))) < 0)
> +	      if (compare_tree_int (TYPE_SIZE_UNIT (type), len - unit) < 0)
>  		pedwarn_init (init_loc, 0,
>  			      ("initializer-string for array of chars "
>  			       "is too long"));
> @@ -7456,8 +7454,21 @@ digest_init (location_t init_loc, tree type, tree
>  		warning_at (init_loc, OPT_Wc___compat,
>  			    ("initializer-string for array chars "
>  			     "is too long for C++"));
> +	      if (compare_tree_int (TYPE_SIZE_UNIT (type), len) < 0)
> +		{
> +		  unsigned HOST_WIDE_INT size
> +		    = tree_to_uhwi (TYPE_SIZE_UNIT (type));
> +		  const char *p = TREE_STRING_POINTER (inside_init);
> +		  char *q = (char *)xmalloc (size + unit);

I guess you want XALLOCAVAR or XNEWVAR.

Marek

Re: [PATCH] Handle overlength strings in the C FE

[Joseph Myers]

On Wed, 1 Aug 2018, Marek Polacek wrote:

> I guess you want XALLOCAVAR or XNEWVAR.

Not XALLOCAVAR; GCC supports string constants up to 2 GB (minus one byte), 
which is far too much to put on the stack.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Handle overlength strings in the C FE

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

On 08/01/18 22:43, Joseph Myers wrote:
> On Wed, 1 Aug 2018, Marek Polacek wrote:
> 
>> I guess you want XALLOCAVAR or XNEWVAR.
> 
> Not XALLOCAVAR; GCC supports string constants up to 2 GB (minus one byte),
> which is far too much to put on the stack.
> 

I assume you want me to use XNEWVEC/XDELETEVEC.

Attached is a new version using those macros.
Is it OK for trunk?


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-c-fe.diff --]
[-- Type: text/x-patch; name="patch-c-fe.diff", Size: 1919 bytes --]

2018-08-01  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-typeck.c (digest_init): Shorten overlength strings.

Index: gcc/c/c-typeck.c
===================================================================
--- gcc/c/c-typeck.c	(revision 263072)
+++ gcc/c/c-typeck.c	(working copy)
@@ -7435,19 +7435,17 @@ digest_init (location_t init_loc, tree type, tree
 		}
 	    }
 
-	  TREE_TYPE (inside_init) = type;
 	  if (TYPE_DOMAIN (type) != NULL_TREE
 	      && TYPE_SIZE (type) != NULL_TREE
 	      && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST)
 	    {
 	      unsigned HOST_WIDE_INT len = TREE_STRING_LENGTH (inside_init);
+	      unsigned unit = TYPE_PRECISION (typ1) / BITS_PER_UNIT;
 
 	      /* Subtract the size of a single (possibly wide) character
 		 because it's ok to ignore the terminating null char
 		 that is counted in the length of the constant.  */
-	      if (compare_tree_int (TYPE_SIZE_UNIT (type),
-				    (len - (TYPE_PRECISION (typ1)
-					    / BITS_PER_UNIT))) < 0)
+	      if (compare_tree_int (TYPE_SIZE_UNIT (type), len - unit) < 0)
 		pedwarn_init (init_loc, 0,
 			      ("initializer-string for array of chars "
 			       "is too long"));
@@ -7456,8 +7454,21 @@ digest_init (location_t init_loc, tree type, tree
 		warning_at (init_loc, OPT_Wc___compat,
 			    ("initializer-string for array chars "
 			     "is too long for C++"));
+	      if (compare_tree_int (TYPE_SIZE_UNIT (type), len) < 0)
+		{
+		  unsigned HOST_WIDE_INT size
+		    = tree_to_uhwi (TYPE_SIZE_UNIT (type));
+		  const char *p = TREE_STRING_POINTER (inside_init);
+		  char *q = XNEWVEC (char, size + unit);
+
+		  memcpy (q, p, size);
+		  memset (q + size, 0, unit);
+		  inside_init = build_string (size + unit, q);
+		  XDELETEVEC (q);
+		}
 	    }
 
+	  TREE_TYPE (inside_init) = type;
 	  return inside_init;
 	}
       else if (INTEGRAL_TYPE_P (typ1))

Re: [PATCH] Handle overlength strings in the C FE

[Joseph Myers]

On Thu, 9 Aug 2018, Bernd Edlinger wrote:

> On 08/01/18 22:43, Joseph Myers wrote:
> > On Wed, 1 Aug 2018, Marek Polacek wrote:
> > 
> >> I guess you want XALLOCAVAR or XNEWVAR.
> > 
> > Not XALLOCAVAR; GCC supports string constants up to 2 GB (minus one byte),
> > which is far too much to put on the stack.
> > 
> 
> I assume you want me to use XNEWVEC/XDELETEVEC.
> 
> Attached is a new version using those macros.
> Is it OK for trunk?

This version is OK.

-- 
Joseph S. Myers
joseph@codesourcery.com

[PATCHv2] Handle overlength strings in the C FE

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 370 bytes --]

Hi!


This is an alternative approach handle overlength strings in the C FE.

The difference to the previous version is that overlength
STRING_CST never have a longer TREE_STRING_LENGTH than the TYPE_DOMAIN.
And those STRING_CSTs are thus no longer zero terminated.

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


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-c-fe-v2.diff --]
[-- Type: text/x-patch; name="patch-c-fe-v2.diff", Size: 1784 bytes --]

2018-08-01  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-typeck.c (digest_init): Shorten overlength strings.

Index: gcc/c/c-typeck.c
===================================================================
--- gcc/c/c-typeck.c	(revision 263807)
+++ gcc/c/c-typeck.c	(working copy)
@@ -7488,19 +7488,17 @@ digest_init (location_t init_loc, tree type, tree
 		}
 	    }
 
-	  TREE_TYPE (inside_init) = type;
 	  if (TYPE_DOMAIN (type) != NULL_TREE
 	      && TYPE_SIZE (type) != NULL_TREE
 	      && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST)
 	    {
 	      unsigned HOST_WIDE_INT len = TREE_STRING_LENGTH (inside_init);
+	      unsigned unit = TYPE_PRECISION (typ1) / BITS_PER_UNIT;
 
 	      /* Subtract the size of a single (possibly wide) character
 		 because it's ok to ignore the terminating null char
 		 that is counted in the length of the constant.  */
-	      if (compare_tree_int (TYPE_SIZE_UNIT (type),
-				    (len - (TYPE_PRECISION (typ1)
-					    / BITS_PER_UNIT))) < 0)
+	      if (compare_tree_int (TYPE_SIZE_UNIT (type), len - unit) < 0)
 		pedwarn_init (init_loc, 0,
 			      ("initializer-string for array of chars "
 			       "is too long"));
@@ -7509,8 +7507,17 @@ digest_init (location_t init_loc, tree type, tree
 		warning_at (init_loc, OPT_Wc___compat,
 			    ("initializer-string for array chars "
 			     "is too long for C++"));
+	      if (compare_tree_int (TYPE_SIZE_UNIT (type), len) < 0)
+		{
+		  unsigned HOST_WIDE_INT size
+		    = tree_to_uhwi (TYPE_SIZE_UNIT (type));
+		  const char *p = TREE_STRING_POINTER (inside_init);
+
+		  inside_init = build_string (size, p);
+		}
 	    }
 
+	  TREE_TYPE (inside_init) = type;
 	  return inside_init;
 	}
       else if (INTEGRAL_TYPE_P (typ1))

Re: [PATCHv2] Handle overlength strings in the C FE

[Jeff Law]

On 8/24/18 1:59 PM, Bernd Edlinger wrote:
> Hi!
> 
> 
> This is an alternative approach handle overlength strings in the C FE.
> 
> The difference to the previous version is that overlength
> STRING_CST never have a longer TREE_STRING_LENGTH than the TYPE_DOMAIN.
> And those STRING_CSTs are thus no longer zero terminated.
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
> 
> 
> Thanks
> Bernd.
> 
> 
> patch-c-fe-v2.diff
> 
> 2018-08-01  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* c-typeck.c (digest_init): Shorten overlength strings.
I've installed this on the the trunk.  As we know this will regress
pr87053 temporarily.

Jeff

Re: [PATCH] Handle overlength strings in the C FE

[Martin Sebor]

On 08/01/2018 05:20 AM, Bernd Edlinger wrote:
> On 07/30/18 17:49, Joseph Myers wrote:
>> On Mon, 30 Jul 2018, Bernd Edlinger wrote:
>>
>>> Hi,
>>>
>>> this is how I would like to handle the over length strings issue in the C FE.
>>> If the string constant is exactly the right length and ends in one explicit
>>> NUL character, shorten it by one character.
>>
>> I don't think shortening should be limited to that case.  I think the case
>> where the constant is longer than that (and so gets an unconditional
>> pedwarn) should also have it shortened - any constant that doesn't fit in
>> the object being initialized should be shortened to fit, whether diagnosed
>> or not, we should define GENERIC / GIMPLE to disallow too-large string
>> constants in initializers, and should add an assertion somewhere in the
>> middle-end that no too-large string constants reach it.
>>
>
> Okay, there is an update following your suggestion.
>
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?

The ChangeLog description says:

	* c-typeck.c (digest_init): Fix overlength strings.

suggesting there is a bug but there is no test case.  If there
is a bug in there that can be triggered by C code (valid or
otherwise), it would be good to have a test case and a bug
in Bugzilla.  If there is no bug and this is just cleanup,
I would suggest to adjust the description.

Other than that, while making improvements here, I think it
would be helpful to also add more detail to the text of
the warning:

1) mention the type of the array being initialized in case
it's not obvious from the declaration (the array bound could
be a symbol, not a literal, or the type could be a typedef)

2) mention the number of elements in the initializer in case
it's a macro (such as __FILE__) whose definition isn't visible
in the diagnostic

3) mention that the excess elements are ignored (since it's
undefined in the standard, it will let users know what
happens in GCC).

Here's a test case and a suggested warning:

   #define S __FILE__ "\000"
   enum { N = sizeof __FILE__ };
   const char a[N] = S;

   warning: discarding 1 excess element from initializer-string for 
'char[4]' [-Wc++-compat]
    #define S __FILE__ "\000"
              ^~~~~~~~
   note: in expansion of macro ‘S’
    const char a[N] = S;
                      ^
(Similarly for more than 1 excess element.)

Martin

Re: [PATCH] Handle overlength strings in the C FE

[Bernd Edlinger]

On 08/01/18 19:07, Martin Sebor wrote:
> On 08/01/2018 05:20 AM, Bernd Edlinger wrote:
>> On 07/30/18 17:49, Joseph Myers wrote:
>>> On Mon, 30 Jul 2018, Bernd Edlinger wrote:
>>>
>>>> Hi,
>>>>
>>>> this is how I would like to handle the over length strings issue in the C FE.
>>>> If the string constant is exactly the right length and ends in one explicit
>>>> NUL character, shorten it by one character.
>>>
>>> I don't think shortening should be limited to that case.  I think the case
>>> where the constant is longer than that (and so gets an unconditional
>>> pedwarn) should also have it shortened - any constant that doesn't fit in
>>> the object being initialized should be shortened to fit, whether diagnosed
>>> or not, we should define GENERIC / GIMPLE to disallow too-large string
>>> constants in initializers, and should add an assertion somewhere in the
>>> middle-end that no too-large string constants reach it.
>>>
>>
>> Okay, there is an update following your suggestion.
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
> 
> The ChangeLog description says:
> 
>      * c-typeck.c (digest_init): Fix overlength strings.
> 
> suggesting there is a bug but there is no test case.  If there
> is a bug in there that can be triggered by C code (valid or
> otherwise), it would be good to have a test case and a bug
> in Bugzilla.  If there is no bug and this is just cleanup,
> I would suggest to adjust the description.
> 

Yes, thanks for looking at this.  This is an attempt to
reduce the number of different encodings for otherwise
identical strings in the middle-end.

I could say "Shorten overlength strings." is that better?
There are already numerous test cases with overlength strings.
I verified that, because I have tested this patch on top
of https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00050.html

> Other than that, while making improvements here, I think it
> would be helpful to also add more detail to the text of
> the warning:
> 

Sure, but it is important to do only one thing at a time.
I see this as a preparation to fixing the remaining
string_constant folding issues which are potential wrong-code
issues.

> 1) mention the type of the array being initialized in case
> it's not obvious from the declaration (the array bound could
> be a symbol, not a literal, or the type could be a typedef)
> 
> 2) mention the number of elements in the initializer in case
> it's a macro (such as __FILE__) whose definition isn't visible
> in the diagnostic
> 
> 3) mention that the excess elements are ignored (since it's
> undefined in the standard, it will let users know what
> happens in GCC).
> 
> Here's a test case and a suggested warning:
> 
>    #define S __FILE__ "\000"
>    enum { N = sizeof __FILE__ };
>    const char a[N] = S;
> 
>    warning: discarding 1 excess element from initializer-string for 'char[4]' [-Wc++-compat]
>     #define S __FILE__ "\000"
>               ^~~~~~~~
>    note: in expansion of macro ‘S’
>     const char a[N] = S;
>                       ^
> (Similarly for more than 1 excess element.)
> 

Yes, definitely helpful, but not part of this patch.
Probably one of your next patches, I would assume.

Bernd.

Re: [PATCH] Handle overlength strings in the C FE

[Eric Gallager]

On 8/1/18, Martin Sebor <msebor@gmail.com> wrote:
> On 08/01/2018 05:20 AM, Bernd Edlinger wrote:
>> On 07/30/18 17:49, Joseph Myers wrote:
>>> On Mon, 30 Jul 2018, Bernd Edlinger wrote:
>>>
>>>> Hi,
>>>>
>>>> this is how I would like to handle the over length strings issue in the
>>>> C FE.
>>>> If the string constant is exactly the right length and ends in one
>>>> explicit
>>>> NUL character, shorten it by one character.
>>>
>>> I don't think shortening should be limited to that case.  I think the
>>> case
>>> where the constant is longer than that (and so gets an unconditional
>>> pedwarn) should also have it shortened - any constant that doesn't fit
>>> in
>>> the object being initialized should be shortened to fit, whether
>>> diagnosed
>>> or not, we should define GENERIC / GIMPLE to disallow too-large string
>>> constants in initializers, and should add an assertion somewhere in the
>>> middle-end that no too-large string constants reach it.
>>>
>>
>> Okay, there is an update following your suggestion.
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
>
> The ChangeLog description says:
>
> 	* c-typeck.c (digest_init): Fix overlength strings.
>
> suggesting there is a bug but there is no test case.  If there
> is a bug in there that can be triggered by C code (valid or
> otherwise), it would be good to have a test case and a bug
> in Bugzilla.  If there is no bug and this is just cleanup,
> I would suggest to adjust the description.
>
> Other than that, while making improvements here, I think it
> would be helpful to also add more detail to the text of
> the warning:
>
> 1) mention the type of the array being initialized in case
> it's not obvious from the declaration (the array bound could
> be a symbol, not a literal, or the type could be a typedef)
>
> 2) mention the number of elements in the initializer in case
> it's a macro (such as __FILE__) whose definition isn't visible
> in the diagnostic
>
> 3) mention that the excess elements are ignored (since it's
> undefined in the standard, it will let users know what
> happens in GCC).
>
> Here's a test case and a suggested warning:
>
>    #define S __FILE__ "\000"
>    enum { N = sizeof __FILE__ };
>    const char a[N] = S;
>
>    warning: discarding 1 excess element from initializer-string for
> 'char[4]' [-Wc++-compat]
>     #define S __FILE__ "\000"
>               ^~~~~~~~
>    note: in expansion of macro ‘S’
>     const char a[N] = S;
>                       ^
> (Similarly for more than 1 excess element.)
>
> Martin
>

While modifying -Woverlength-strings, I think a good way to test it
would be to enable it during bootstrap. Even though it is currently
disabled, I have done bootstraps before while manually enabling it,
and they still succeeded. Plus there is documentation that says to
avoid overlength strings in GCC sources, so it would be good if this
were verified. I also brought this up in bug 80942:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80942

Eric

Re: [PATCH] Handle overlength strings in the C FE

[Joseph Myers]

On Wed, 1 Aug 2018, Eric Gallager wrote:

> While modifying -Woverlength-strings, I think a good way to test it

The -Woverlength-strings option (warn about string constants longer than 
the minimum guaranteed by the C standard to be supported) has nothing 
whatever to do with the present patch discussion (which relates to string 
constants longer than the array they are used as an initializer for).

-- 
Joseph S. Myers
joseph@codesourcery.com