On 28/12/2023 18:56, René Scharfe wrote:
Am 28.12.23 um 17:48 schrieb phillip.wood123@xxxxxxxxx:
On 28/12/2023 16:05, René Scharfe wrote:
Am 28.12.23 um 16:10 schrieb Phillip Wood:
The diff at the end of
this email shows a possible implementation of a check_ptr() macro for
the unit test library. I'm wary of adding it though because I'm not sure
printing the pointer values is actually very useful most of the
time. I'm also concerned that the rules around pointer arithmetic and
comparisons mean that many pointer tests such as
check_ptr(pool->mp_block->next_free, <=, pool->mp_block->end);
will be undefined if they fail.
True, the compiler could legally emit mush when it finds out that the
pointers are for different objects. And the error being fixed produces
such unrelated pointer pairs -- oops.
This check is not important here, we can just drop it.
mem_pool_contains() has the same problem, by the way.
Restricting ourselves to only equality comparisons for pointers prevents
some interesting sanity checks, though. Casting to intptr_t or
uintptr_t would allow arbitrary comparisons without risk of undefined
behavior, though. Perhaps that would make a check_ptr() macro viable
and useful.
That certainly helps and the check_ptr() macro in my previous email
casts the pointers to uintptr_t before comparing them. Maybe I'm
worrying too much, but my concern is that in a failing comparison it
is likely one of the pointers is invalid (for example it is the
result of some undefined pointer arithmetic) and the program is
undefined from the point the invalid pointer is created.
There are no restrictions on integer comparisons. So comparing after
casting to uintptr_t should not invoke undefined behavior. If undefined
behavior was involved in calculating the pointers in the first place
then the compiler might still legally go crazy, but not due to the
comparison. Right?
Exactly, my worry is that if the comparison fails it is likely that
there will have been undefined behavior involved in calculating the
pointer before we get to the comparison in which case so casting to
uintptr_t in the comparison does not help.
Whether the result of a uintptr_t-cast comparison of pointers to
different objects is meaningful is a different question. Hopefully
range checks are possible.
The
documentation for check_ptr() in my previous mail contains the
following example
For example if `start` and `end` are pointers to the beginning and
end of an allocation and `offset` is an integer then
check_ptr(start + offset, <=, end)
is undefined when `offset` is larger than `end - start`. Rewriting
the comparison as
check_uint(offset, <=, end - start)
avoids undefined behavior when offset is too large, but is still
undefined if there is a bug that means `start` and `end` do not
point to the same allocation.
True, but in such a unit test we'd need additional checks verifying
that start and end belong to the same object. Or perhaps use a
numerical size instead of an end pointer.
Agreed, but I think the implication is that there will be cases we
should be using check_uint() as in the second comparison above rather
than check_ptr() as in the first comparison above. I'm not opposed to
adding check_ptr() if we think it will be useful but I am worried it is
easy to misuse it. If we do add check_ptr() we should have some
guidelines about when it makes sense to use it.
Best Wishes
Phillip
