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