Re: [PATCH] have -Wformat-overflow handle -fexec-charset (PR 80503)

[Jeff Law <law@redhat.com>]

On 04/28/2017 12:05 PM, Martin Sebor wrote:

>> So the initialization could be done once per translation unit rather
>> than once per function -- assuming the target character set doesn't
>> change within a translation unit.
>>
>> That seems like it ought to be easy.
> 
> It is easy.  I was going to respond by saying "It already is done
> that way" because I implemented it in the first patch (by checking
> the mapping for '%'.  But now I see I accidentally removed the code
> in the update while exploring ways to optimize it some more.  Sigh.
> Let me put it back.
:-)

> 
>>> +   set corresponding to the string TARGSTR consisting of characters in
>>> +   the execution character set.  */
>>> +
>>> +static const char*
>>> +target_to_host (const char *targstr)
>>> +{
>>> +  /* The interesting subset of source and execution characters are
>>> +     the same so no conversion is necessary.  */
>>> +  if (target_to_host_charmap['\0'] == 1)
>>> +    return targstr;
>>> +
>>> +  /* Convert the initial substring of TARGSTR to the corresponding
>>> +     characters in the host set, appending "..." if TARGSTR is too
>>> +     long to fit.  Using the static buffer assumes the function is
>>> +     not called in between sequence points (which it isn't).  */
>>> +  static char hostr[32];
>>> +  for (char *ph = hostr; ; ++targstr)
>>> +    {
>>> +      *ph++ = target_to_host (*targstr);
>>> +      if (!*targstr)
>>> +    break;
>>> +
>>> +      if (ph - hostr == sizeof hostr - 4)
>>> +    {
>>> +      *ph = '\0';
>>> +      strcat (ph, "...");
>>> +      break;
>>> +    }
>>> +    }
>>> +
>>> +  return hostr;
>> Ewww.  I guess the alternative would be something like:
>>
>> Expand the return value to include a indicator of whether or not the
>> original string was returned (common case) or if a new string was
>> returned and thus needs to be deallocated by the caller.
>>
>> That's probably pretty unpleasant given we don't have a central place
>> where we call target_to_host, so the caller side would be much uglier.
>>
>> Are there any downstream impacts when the string is too long to covert
>> other than not warning for things which were unconverted?
> 
> The function is only used when printing the text of the directive
> in the warning.  It doesn't prevent the warnings, it just truncates
> them.  
Noted.  Thanks.


> 
> I don't like returning a pointer to a static buffer but given that
> the scope of the function is limited to the pass it seems fairly
> safe.  Another alternative would be to pass in a buffer and its
> size.  That shouldn't complicate the caller too much.  The easiest,
> cleanest, and safest solution by far is to return a std::string,
> but I have the impression that would be against GCC convention of
> avoiding the STL.
That's what caught my eye.  I don't generally like that either. 
Putting the buffer into the caller is better.  Thanks for doing that.




> 
> Attached is an updated patch with these two tweaks.
> 
> Martin
> 
> gcc-80523.diff
> 
> 
> PR tree-optimization/80523 -  -Wformat-overflow doesn't consider -fexec-charset
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/80523
> 	* gimple-ssa-sprintf.c (target_to_host_charmap): New global variable.
> 	(init_target_to_host_charmap, target_to_host, target_strtol10): New
> 	functions.
> 	(maybe_warn, format_directive, parse_directive): Use new functions.
> 	(pass_sprintf_length::execute): Call init_target_to_host_charmap.	
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/80523
> 	* gcc.dg/tree-ssa/builtin-sprintf-warn-18.c: New test.
OK.

jeff