Copilot commented on code in PR #13170:
URL: https://github.com/apache/trafficserver/pull/13170#discussion_r3405445142
##########
src/tscore/ink_queue.cc:
##########
@@ -263,10 +319,11 @@ freelist_new(InkFreeList *f)
#endif
freelist_free(f, a);
}
-
} else {
- SET_FREELIST_POINTER_VERSION(next,
*ADDRESS_OF_NEXT(TO_PTR(FREELIST_POINTER(item)), 0), FREELIST_VERSION(item) +
1);
- result = ink_atomic_cas(&f->head.data, item.data, next.data);
+ void **next_ptr = reinterpret_cast<void
**>(TO_PTR(FREELIST_POINTER(item)));
+
+ SET_FREELIST_POINTER_VERSION(next, *next_ptr, FREELIST_VERSION(item) +
1);
Review Comment:
`freelist_new()` dereferences the node's next pointer via
`reinterpret_cast<void **>` without using the new `to_voidp_p()` helper, so the
alignment assertion you added there is bypassed. Using `to_voidp_p()` here
keeps the alignment check consistent and avoids UB if an unexpected
misalignment ever slips through.
##########
src/tscore/ink_queue.cc:
##########
@@ -517,76 +600,96 @@ ink_freelists_dump(FILE *f)
void
ink_atomiclist_init(InkAtomicList *l, const char *name, uint32_t
offset_to_next)
{
+ // The pointers we push onto the atomiclist will also need to be aligned. If
+ // the offset is not aligned, then it is not possible for the caller to
+ // determine a consistent, safe alignment for the object the void* objects
+ // are subobjects of.
+ ink_release_assert(offset_to_next % alignof(void *) == 0);
+
l->name = name;
l->offset = offset_to_next;
- SET_FREELIST_POINTER_VERSION(l->head, FROM_PTR(0), 0);
+ head_p empty_head;
+ SET_FREELIST_POINTER_VERSION(empty_head, FROM_PTR(nullptr), 0);
+ l->head.store(empty_head);
}
void *
ink_atomiclist_pop(InkAtomicList *l)
{
head_p item;
head_p next;
- int result = 0;
+ bool result = 0;
+
+ std::lock_guard guard{l->m};
+
+ item = l->head.load();
do {
- INK_QUEUE_LD(item, l->head);
if (TO_PTR(FREELIST_POINTER(item)) == nullptr) {
return nullptr;
}
- SET_FREELIST_POINTER_VERSION(next,
*ADDRESS_OF_NEXT(TO_PTR(FREELIST_POINTER(item)), l->offset),
FREELIST_VERSION(item) + 1);
- result = ink_atomic_cas(&l->head.data, item.data, next.data);
- } while (result == 0);
- {
- void *ret = TO_PTR(FREELIST_POINTER(item));
- *ADDRESS_OF_NEXT(ret, l->offset) = nullptr;
- return ret;
- }
+ void **next_ptr = reinterpret_cast<void **>(reinterpret_cast<unsigned char
*>(TO_PTR(FREELIST_POINTER(item))) + l->offset);
+ SET_FREELIST_POINTER_VERSION(next, *next_ptr, FREELIST_VERSION(item) + 1);
+ result = l->head.compare_exchange_weak(item, next);
+ } while (result == false);
+
+ void *ret = TO_PTR(FREELIST_POINTER(item));
+ void **ret_ = reinterpret_cast<void **>(reinterpret_cast<unsigned char
*>(ret) + l->offset);
+ *ret_ = nullptr;
Review Comment:
After popping an item in `ink_atomiclist_pop()`, clearing the embedded
next-pointer uses another unchecked `reinterpret_cast<void **>`; using
`to_voidp_p()` keeps the alignment assertion in one place and avoids repeating
the pointer arithmetic.
##########
src/tscore/ink_queue.cc:
##########
@@ -517,76 +600,96 @@ ink_freelists_dump(FILE *f)
void
ink_atomiclist_init(InkAtomicList *l, const char *name, uint32_t
offset_to_next)
{
+ // The pointers we push onto the atomiclist will also need to be aligned. If
+ // the offset is not aligned, then it is not possible for the caller to
+ // determine a consistent, safe alignment for the object the void* objects
+ // are subobjects of.
+ ink_release_assert(offset_to_next % alignof(void *) == 0);
+
l->name = name;
l->offset = offset_to_next;
- SET_FREELIST_POINTER_VERSION(l->head, FROM_PTR(0), 0);
+ head_p empty_head;
+ SET_FREELIST_POINTER_VERSION(empty_head, FROM_PTR(nullptr), 0);
+ l->head.store(empty_head);
}
void *
ink_atomiclist_pop(InkAtomicList *l)
{
head_p item;
head_p next;
- int result = 0;
+ bool result = 0;
+
+ std::lock_guard guard{l->m};
+
+ item = l->head.load();
do {
- INK_QUEUE_LD(item, l->head);
if (TO_PTR(FREELIST_POINTER(item)) == nullptr) {
return nullptr;
}
- SET_FREELIST_POINTER_VERSION(next,
*ADDRESS_OF_NEXT(TO_PTR(FREELIST_POINTER(item)), l->offset),
FREELIST_VERSION(item) + 1);
- result = ink_atomic_cas(&l->head.data, item.data, next.data);
- } while (result == 0);
- {
- void *ret = TO_PTR(FREELIST_POINTER(item));
- *ADDRESS_OF_NEXT(ret, l->offset) = nullptr;
- return ret;
- }
+ void **next_ptr = reinterpret_cast<void **>(reinterpret_cast<unsigned char
*>(TO_PTR(FREELIST_POINTER(item))) + l->offset);
+ SET_FREELIST_POINTER_VERSION(next, *next_ptr, FREELIST_VERSION(item) + 1);
Review Comment:
`ink_atomiclist_pop()` computes `next_ptr` with manual pointer arithmetic
and `reinterpret_cast<void **>`, bypassing the `to_voidp_p()` alignment
assertion. Using `to_voidp_p()` here would make the access rules consistent
with `ink_atomiclist_popall()` and catch misaligned offsets/items earlier in
debug builds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
