This is the mail archive of the 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: PING [PATCH] warn for strlen of arrays with missing nul (PR 86552, 86711, 86714) )

On 08/25/18 21:02, Jeff Law wrote:
> On 08/25/2018 12:36 PM, Bernd Edlinger wrote:
>>>> Well, ya call it "layer one patch over the other"
>>>> I call it "incremental improvements".
>>> It is (of course) a case by case basis.  The way I try to look at these
>>> things is to ask whether or not the first patch under consideration
>>> would have any value/purpose after the second patch was installed.  If
>>> so, then it may make sense to include both.  If not, then we really just
>>> want one patch.
>> Agreed.  I think the question is which of the possible STRING_CST
>> semantics we want to have in the end (the middle-end).
>> Everything builds on top of the semantic properties of STRING_CSTs.
> This certainly plays a role.  I bumped pretty hard against the
> STRING_CST semantics issue with Martin's patch.  I'm hoping that making
> those more consistent will ultimately simplify things and avoid the
> problems I'm stumbling over.
> Of course, that means more delays in getting this sorted out.  I really
> thought I had a viable plan a couple days ago, but I'm having to rethink
> in light of some of the issues raised.

I think we should slow down.

>> My first attempt of fix the STRING_CST semantic was trying to make
>> string_constant happy.
>> My second attempt is trying to make Richard happy.  And when I look
>> at both patches, I think the second one is better, and more simple.
> In general I've found that Richie's advice generally results in a
> cleaner implementation ;-)
>> BTW I need to correct on statement in my last e-mail:
>> On 08/24/18 23:55, Bernd Edlinger wrote:>
>>> here I quote form martins 1/6 patch:
>>>> +  /* Compute the lower bound number of elements (not bytes) in the array
>>>> +     that the string is used to initialize.  The actual size of the array
>>>> +     will be may be greater if the string is shorter, but the important
>>>> +     data point is whether the literal, including the terminating nul,
>>>> +     fits in the array. */
>>>> +  unsigned HOST_WIDE_INT array_elts
>>>> +    = tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (init))) / charsize;
>>>> +
>>>> +  /* Compute the string length in (wide) characters.  */
>>> So prior to my STRING_CST patch this will ICE on a flexible array member,
>>> because those have TYPE_SIZE_UNIT = NULL, and tree_to_uhwi will ICE.
>>> I used:
>>>    compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (init)),
>>>                      TREE_STRING_LENGTH (init))
>>> and this will not ICE with NULL, but consider it like infinity,
>>> and return 1.
>>> So my version did not ICE in that case.
>> Oooops,
>> actually both versions will likely ICE on TYPE_SIZE_UNIT == NULL.
>> I actually tried to test that case, but have done something wrong.
>> I hope we can get rid if the incomplete types in the middle-end.
>> So maybe just inject "if (!tree_fits_uhwi_p (...)) return NULL_TREE;"
>> here?   Or maybe just defer until we have clarity about the semantics.
> Not sure.  I've tried largely to not let VLA issues drive anything here.
>   I'm not a fan of them for a variety of reasons and thus I tend to look
> at all the VLA stuff as exceptional cases.

We should have a test case with flexible array members, those behave
slightly different than VLAs.

struct {
   int i;
   char x[];
} s;

const struct s s = { 1, "test" };

int f()
   return strlen(s.x);

By the way, when you change so much on Martin's patch it might be
good to post it again on this list, when it's ready, so maybe I can have a
look at it and help out with some review comments?  (please put me on CC)


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