This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, obvious?] Some minor nits in string folding functions


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.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]