[obvious?] Some minor nits in string folding functions

Message ID AM5PR0701MB2657279EAD8AE697C7826648E4520@AM5PR0701MB2657.eurprd07.prod.outlook.com
State New
Headers show
Series
  • [obvious?] Some minor nits in string folding functions
Related show

Commit Message

Bernd Edlinger July 19, 2018, 6:04 p.m.
Hi,


this fixes a few minor nits, which I spotted while
looking at the string folding functions:

string_constant: Remove impossible check: TREE_CODE (arg)
can't be COMPONENT_REF and MEM_REF at the same time.

c_strlen: maxelts is (signed) HOST_WIDE_INT, therefore
use tree_to_shwi.

c_getstr: tree_to_uhwi needs to be protected by
tree_fits_uhwi_p.

BTW: c_getstr uses string_constant which appears to be
able to extract wide char string initializers, but c_getstr
does not seem to be prepared for wide char strings:

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


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


Thanks
Bernd.

Comments

Jeff Law July 19, 2018, 6:11 p.m. | #1
On 07/19/2018 12:04 PM, Bernd Edlinger wrote:
> Hi,

> 

> 

> this fixes a few minor nits, which I spotted while

> looking at the string folding functions:

> 

> string_constant: Remove impossible check: TREE_CODE (arg)

> can't be COMPONENT_REF and MEM_REF at the same time.

Shouldn't they all be != tests?  Though clearly this code isn't being
tested by anything.


> 

> c_strlen: maxelts is (signed) HOST_WIDE_INT, therefore

> use tree_to_shwi.

One could argue maxelts should be unsigned.

> 

> c_getstr: tree_to_uhwi needs to be protected by

> tree_fits_uhwi_p.

Looks correct to me.


> 

> BTW: c_getstr uses string_constant which appears to be

> able to extract wide char string initializers, but c_getstr

> does not seem to be prepared for wide char strings:

> 

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

>      }

Seems like a goof to me.

jeff

Jeff
Bernd Edlinger July 19, 2018, 7:25 p.m. | #2
On 07/19/18 20:11, Jeff Law wrote:
> On 07/19/2018 12:04 PM, Bernd Edlinger wrote:

>> Hi,

>>

>>

>> this fixes a few minor nits, which I spotted while

>> looking at the string folding functions:

>>

>> string_constant: Remove impossible check: TREE_CODE (arg)

>> can't be COMPONENT_REF and MEM_REF at the same time.

> Shouldn't they all be != tests?  Though clearly this code isn't being

> tested by anything.

> 


I think a COMPONENT_REF would be possible for strlen(&a.b).
It looks like that removing this check will be the best option.
Unless Martin has a different idea of course.

> 

>>

>> c_strlen: maxelts is (signed) HOST_WIDE_INT, therefore

>> use tree_to_shwi.

> One could argue maxelts should be unsigned.

> 


I would agree, but a few lines later I see:

   /* Offset from the beginning of the string in elements.  */
   HOST_WIDE_INT eltoff;

   /* We have a known offset into the string.  Start searching there for
      a null character if we can represent it as a single HOST_WIDE_INT.  */
   if (byteoff == 0)
     eltoff = 0;
   else if (! tree_fits_shwi_p (byteoff))
     eltoff = -1;
   else
     eltoff = tree_to_shwi (byteoff) / eltsize;

   /* If the offset is known to be out of bounds, warn, and call strlen at
      runtime.  */
   if (eltoff < 0 || eltoff > maxelts)
     {

This would be doing signed/unsigned comparisons then.
Maybe that is the reason why ?

>>

>> c_getstr: tree_to_uhwi needs to be protected by

>> tree_fits_uhwi_p.

> Looks correct to me.

> 

> 

>>

>> BTW: c_getstr uses string_constant which appears to be

>> able to extract wide char string initializers, but c_getstr

>> does not seem to be prepared for wide char strings:

>>

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

>>       }

> Seems like a goof to me.

> 


Well, maybe this could be a gcc_assert instead.  Normal strings should always
be zero-terminated, even wide character strings.
Anyways probably for another time.


Bernd.
Martin Sebor July 19, 2018, 7:37 p.m. | #3
On 07/19/2018 12:04 PM, Bernd Edlinger wrote:
> Hi,

>

>

> this fixes a few minor nits, which I spotted while

> looking at the string folding functions:


Please hold off until the patch for bug 86532 has been reviewed,
approved, and committed.  I'm making changes in this area,
partly to address some of your comments on it, including some
of the same ones you are making here.  It doesn't help for you
to be making other changes to the same code at the same time.

Thanks
Martin

>

> string_constant: Remove impossible check: TREE_CODE (arg)

> can't be COMPONENT_REF and MEM_REF at the same time.

>

> c_strlen: maxelts is (signed) HOST_WIDE_INT, therefore

> use tree_to_shwi.

>

> c_getstr: tree_to_uhwi needs to be protected by

> tree_fits_uhwi_p.

>

> BTW: c_getstr uses string_constant which appears to be

> able to extract wide char string initializers, but c_getstr

> does not seem to be prepared for wide char strings:

>

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

>      }

>

>

> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.

> Is it OK for trunk?

>

>

> Thanks

> Bernd.

>

Patch

2018-07-19  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* builtins.c (c_strlen): Use tree_to_shwi.
	* expr.c (string_constant): Remove impossible check.
	* fold-const.c (c_getstr): Use tree_fits_uhwi_p.

Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c	(revision 262861)
+++ gcc/builtins.c	(working copy)
@@ -610,7 +610,7 @@  c_strlen (tree src, int only_value)
   tree type = TREE_TYPE (src);
   if (tree size = TYPE_SIZE_UNIT (type))
     if (tree_fits_shwi_p (size))
-      maxelts = tree_to_uhwi (size);
+      maxelts = tree_to_shwi (size);
 
   maxelts = maxelts / eltsize - 1;
 
Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 262861)
+++ gcc/expr.c	(working copy)
@@ -11371,11 +11371,6 @@  string_constant (tree arg, tree *ptr_offset)
     return NULL_TREE;
   if (TREE_CODE (init) == CONSTRUCTOR)
     {
-      if (TREE_CODE (arg) != ARRAY_REF
-	  && TREE_CODE (arg) == COMPONENT_REF
-	  && TREE_CODE (arg) == MEM_REF)
-	return NULL_TREE;
-
       /* Convert the 64-bit constant offset to a wider type to avoid
 	 overflow.  */
       offset_int wioff;
Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c	(revision 262861)
+++ gcc/fold-const.c	(working copy)
@@ -14613,7 +14613,7 @@  c_getstr (tree src, unsigned HOST_WIDE_INT *strlen
   unsigned HOST_WIDE_INT string_size = string_length;
   tree type = TREE_TYPE (src);
   if (tree size = TYPE_SIZE_UNIT (type))
-    if (tree_fits_shwi_p (size))
+    if (tree_fits_uhwi_p (size))
       string_size = tree_to_uhwi (size);
 
   if (strlen)