> On Nov 20, 2024, at 14:23, Martin Uecker <uec...@tugraz.at> wrote:
> 
> Am Mittwoch, dem 20.11.2024 um 17:37 +0000 schrieb Qing Zhao:
>> Hi, Martin,
>> 
>> Thanks a lot for pointing this out. 
>> 
>> This does look like a problem we need avoid for the pointer arrays.
>> 
>> Does  the same problem exist in the language extension too if the n is 
>> allowed to be changed after initialization?
>> 
>> If so, for the future language extension, is there any proposed solution to 
>> this problem? 
>> 
> 
> There is no specification yet and nothing formally proposed, so
> it is entirely unclear at this point.
> 
> My idea would be to give 'x->buf' the type '(char(*)[x->n])'
> where 'x->n' is loaded at the same time 'x->buf' is accessed
> and then the value is frozen (like in the simpler versions
> of 'counted_by' you had implemented first).  Of course, one
> would then have to set x->n before setting the buffer (or
> at the same time). This could be ensured by making the
> member 'n' const, so that it can only be changed by
> overwriting the whole struct. But I am still thinking
> about this.

Okay, so the key here is:  
x->n, x->p can only be changed by changing the whole structure at the same 
time. 

Otherwise, x->n might not be consistent with x->p. 

We might need to add such limitation for the counted-by attribute of pointer 
field in the documentation. 

> 
> In any case, I think for "counted_by" this is not an option
> because it would be confusing if it works differently
> than for the FAM case.

But we need to clearly let the user know the limitation of using counted-by 
attribute for pointer field. 

Qing
> 
> Martin
> 
> 
>> Qing
>>> On Nov 20, 2024, at 10:52, Martin Uecker <uec...@tugraz.at> wrote:
>>> 
>>> Am Mittwoch, dem 20.11.2024 um 15:27 +0000 schrieb Qing Zhao:
>>>> 
>>>>> On Nov 19, 2024, at 10:47, Marek Polacek <pola...@redhat.com> wrote:
>>>>> 
>>>>> On Mon, Nov 18, 2024 at 07:10:35PM +0100, Martin Uecker wrote:
>>>>>> Am Montag, dem 18.11.2024 um 17:55 +0000 schrieb Qing Zhao:
>>>>>>> Hi,
>>>> 
>>> ..
>>> 
>>> Hi Qing,
>>> 
>>>> Per our discussion so far, if treating the following
>>>> 
>>>> struct foo {
>>>> int n;
>>>> char *p __attribute__ ((counted_by (n)));
>>>> };
>>>> 
>>>> as an array with upper-bounds being “n” is reasonable.
>>> 
>>> There is one issue I want to point out, which I just realized during
>>> a discussion in WG14.  For "counted_by" we defined the semantics
>>> in a way that later store to 'n' will be taken into account. 
>>> We did this to support the use case where 'n' is set after
>>> the access to 'p'.
>>> 
>>> struct foo *x = ;
>>> 
>>> char *q = x->p;
>>> x->n = 100; // this should apply
>>> 
>>> 
>>> For FAMs this is fine, because it is a fixed part
>>> of the struct.  But for the new pointer case, I think this is
>>> problematic.  Consider this example:
>>> 
>>> struct foo *x = allocate_buffer(100);
>>> 
>>> where x->n is set to the right value in the allocation function.
>>> 
>>> Now let's continue with
>>> 
>>> char *q = x->p;
>>> 
>>> x = allocate_buffer(50);
>>> // x->n is set to 50.
>>> 
>>> Now q refers to the old buffer, but x->n to the size of the new
>>> buffer.  That does not seem right and scares me a little bit.
>>> 
>>> 
>>> 
>>> Martin
>>> 
>>> 
>>>> 
>>>> Then, it’s reasonable to extend -fsanitize=bounds to instrument the 
>>>> corresponding reference for the pointer with
>>>> Counted-by attribute. 
>>>> 
>>>> What do you think?
>>>> 
>>>> Qing
>>>> 
>>>>> 
>>>>>> I think the question is what -fsanitize=bounds is meant to be.
>>>>>> 
>>>>>> I am a bit frustrated about the sanitizer.  On the
>>>>>> one hand, it is not doing enough to get spatial memory
>>>>>> safety even where this would be easily possible, on the
>>>>>> other hand, is pedantic about things which are technically
>>>>>> UB but not problematic and then one is prevented from
>>>>>> using it
>>>>>> 
>>>>>> When used in default mode, where execution continues, it
>>>>>> also does not mix well with many warning, creates more code,
>>>>>> and pulls in a libary dependency (and the library also depends
>>>>>> on upstream choices / progress which seems a limitation for
>>>>>> extensions).
>>>>>> 
>>>>>> What IMHO would be ideal is a protection mode for spatial
>>>>>> memory safety that simply adds traps (which then requires
>>>>>> no library, has no issues with other warnings, and could
>>>>>> evolve independently from clang) 
>>>>>> 
>>>>>> So shouldn't we just add a -fboundscheck (which would 
>>>>>> be like -fsanitize=bounds -fsanitize-trap=bounds just with
>>>>>> more checking) and make it really good? I think many people
>>>>>> would be very happy about this.
>>>>> 
>>>>> That's a separate concern.  We already have the -fbounds-check option,
>>>>> currently only used in Fortran (and D?), so perhaps we could make
>>>>> that option a shorthand for -fsanitize=bounds -fsanitize-trap=bounds.
>>>>> 
>>>>> Marek
>>>>> 
>>>> 
>>> 
>> 
> 

Reply via email to