Hi, thanks a lot for all the discussion so far on this issue. An update on this:
1. I have reverted the 3 patches to support counted_by for pointers I have committed last week from master. 2. At the same time: On the C FE code generation to .ACCESS_WITH_SIZE for pointers with counted_by attribute, I modified as following: From OLD: _2 = &a->c; _3 = &a->count; _1 = .ACCESS_WITH_SIZE (_2, _3, 1, 0, -1, 0B); _4 = *_1; D.2964 = __builtin_dynamic_object_size (_4, 1); To NEW: _2 = a->c; _3 = &a->count; _1 = .ACCESS_WITH_SIZE (_2, _3, 1, 0, -1, 0B, 0); D.2964 = __builtin_dynamic_object_size (_, 1); NOTE, in the above, in addition to pass “a->c” instead of “&a->c” as the first parameter, I also added one more argument for .ACCESS_WITH_SIZE: + the 7th argument of the call is 1 when for FAM, 0 for pointers. To distinguish whether this .ACCESS_WITH_SIZE is for FAM or for pointers. And this argument will be used in tree-object-size.cc <http://tree-object-size.cc/> to get the element_type of the associated FAM or pointer array. 3. The __builtin_dynamic_object_size works fine with this change. And also PR120929 is fixed with the above change + delete the following from tree-object-size.cc <http://tree-object-size.cc/>: #if 0 /* Handle the following stmt #2 to propagate the size from the stmt #1 to #3: 1 _1 = .ACCESS_WITH_SIZE (_3, _4, 1, 0, -1, 0B); 2 _5 = *_1; 3 _6 = __builtin_dynamic_object_size (_5, 1); */ else if (TREE_CODE (rhs) == MEM_REF && POINTER_TYPE_P (TREE_TYPE (rhs)) && TREE_CODE (TREE_OPERAND (rhs, 0)) == SSA_NAME && integer_zerop (TREE_OPERAND (rhs, 1))) reexamine = merge_object_sizes (osi, var, TREE_OPERAND (rhs, 0)); #endif 4. Bounds sanitizer needs some adjustment due to the IL change, I will continue with bounds sanitizer adjustment. Let me know if you see any issues with my current updating. Thanks a lot for all the help. Qing > On Jul 7, 2025, at 07:48, Jakub Jelinek <ja...@redhat.com> wrote: > > On Mon, Jul 07, 2025 at 07:21:26AM -0400, Siddhesh Poyarekar wrote: >>> is .ACCESS_WITH_SIZE documented? I can't find it documented in the >>> internals manual, internal-fn.def has >> >> It's documented in tree-object-size.cc as: >> >> /* Compute __builtin_object_size for a CALL to .ACCESS_WITH_SIZE, >> OBJECT_SIZE_TYPE is the second argument from __builtin_object_size. >> The 2nd, 3rd, and the 4th parameters of the call determine the size of >> the CALL: >> >> 2nd argument REF_TO_SIZE: The reference to the size of the object, >> 3rd argument CLASS_OF_SIZE: The size referenced by the REF_TO_SIZE >> represents >> 0: the number of bytes; >> 1: the number of the elements of the object type; >> 4th argument TYPE_OF_SIZE: A constant 0 with its TYPE being the same as >> the TYPE >> of the object referenced by REF_TO_SIZE >> 6th argument: A constant 0 with the pointer TYPE to the original flexible >> array type or pointer field type. >> >> The size of the element can be retrived from the TYPE of the 6th argument >> of the call, which is the pointer to the original flexible array type or >> the type of the original pointer field. */ >> >> which doesn't document the return either. This should have more verbose >> documentation in the internals, including the rationale for its existence. >> >>> /* A function to associate the access size and access mode information >>> with the corresponding reference to an object. It only reads from the >>> 2nd argument. */ >>> DEF_INTERNAL_FN (ACCESS_WITH_SIZE, ECF_PURE | ECF_LEAF | ECF_NOTHROW, NULL) >>> >>> that suggests .ACCESS_WITH_SIZE performs a read on the size. It doesn't >>> say what the function returns at all. >> >> In practice the function is a nop, it gets optimized away during RTL >> expansion. The aim is simply to pretend that the reference of the size may >> escape pointer to make sure that any preceding updates to size don't get >> reordered w.r.t. __builtin_dynamic_object_size since the latter could get >> expanded to that size. >> >> The return value of .ACCESS_WITH_SIZE clobbering PTR (that subsequently gets >> passed to __builtin_dynamic_object_size) should be sufficient to fully >> prevent the reordering, it shouldn't have to clobber &PTR, I think. > > The original use of .ACCESS_WITH_SIZE was designed for FAMs, for those > it IMHO does the right thing, it is a pass through first arg function > which attaches size information to the passed as well as returned pointer. > That pointer is &FAM, so address of the containing structure plus offsetof > of the FAM first element. > > The way it is used for non-FAMs looks just wrong. > It passes as first argument the address of the pointer, not the pointer > itself. So we have ifn used for two completely different purposes with > different meanings, while the arguments are otherwise pretty much the same > (or how do you uniquely distinguish the cases where it provides object > size for what it returns vs. where it provides object size for what the > pointer it returns points to). That is like the spaghetti code in certain > middle end warnings. For warnings it is really bad, for code generation > decisions it is a fatal design flaw. > > So, either you need a different ifn, or add some flag in bitfield > that clearly distinguishes the 2 cases, or different number of arguments, > or perhaps most easily, why do the dereference at all? > When I have > struct U { int n; int fam[n] __attribute__((counted_by (n))); } *u; > continue passing &u->fam as first argument and &u->n as second, while for > struct S { > int n; > int (*p)[n] __attribute__((counted_by(n))); > } *f; > don't pass &f->p to the builtin but pass f->p. You are providing size > for f->p pointer, not for &f->p pointer, while for FAM it is for &u->fam > pointer. The second argument would be &f->n. > > So, my recommendation would be to revert the counted_by GCC 16 series, > rework it and submit again. Unless you can fix it up in a day or two. > > Jakub >
