https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98503

Willy Tarreau <w at 1wt dot eu> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|INVALID                     |---
             Status|RESOLVED                    |REOPENED

--- Comment #7 from Willy Tarreau <w at 1wt dot eu> ---
It's not easy because as often with optimizations, depending on the code moves
I'm doing, the issue appears and disappears. The closest looking one is below.

The general idea is that we're having a function that is called to dump a
certain number of elements to a buffer and return when the buffer is full, to
give back control to other parts of the code, then it's called again from the
last list element where the dump was started. A common practice with such
interruptible processing of list consists in starting from the head and letting
the dump function iterate. This would roughly look like this (still simplified
quite a bit to grasp the principle). The code below manages to trigger the
warning. If you're interested in the original code (I doubt it), it's on this
line: https://github.com/haproxy/haproxy/blob/master/src/ssl_sock.c#L6366 and
the initial call is made here:
https://github.com/haproxy/haproxy/blob/master/src/ssl_sock.c#L6437

#include <stddef.h>

#define container_of(ptr, type, member) ({                              \
        void *__mptr = (void *)(ptr);                                   \
        ((type *)(__mptr - __builtin_offsetof(type, member))); })

// returns number of bytes emitted or 0 if it failed to dump.
extern int try_dump_string(const char *name);

struct list {
    struct list *n;
    struct list *p;
};

static struct list head;

struct ref {
        struct list list;
        char *filename;
};

static struct ref *last_dumped;

/* try to dump one ref, returns 0 on failure */
static inline int try_dump_next_ref(struct ref *ref)
{
    return try_dump_string(ref->filename);
}

static inline struct ref *get_initial_step()
{
    return container_of(&head, struct ref, list);
}

static inline struct ref *next_step(struct ref *cur)
{
    return container_of(cur->list.n, struct ref, list);
}

/* restarts a dump from dump_context or starts a new one if NULL */
struct ref *restart_dump(struct ref *last)
{
        struct ref *curr;

        if (!last)
            last = get_initial_step();

        curr = next_step(last);
        while (&curr->list != &head) {
            if (try_dump_next_ref(curr)) {
                last = curr;
                curr = next_step(curr);
            } else {
                /* do something to yield or flush the output */
            }
        }
        return last;
}

void cont_dump()
{
    restart_dump(last_dumped);
}


Please note that here "last_dump" isn't updated and if the compiler sees it
being written to, it eliminates some optimizations that result in the warning
(I think it sees that the get_initial_step() is conditional and thus doesn't
complain anymore).

I understand from your description that there could be an aliasing issue. I'd
argue that as soon as we're using lists there are potential aliasing issues and
that these may then be addressed using -fno-strict-aliasing but this one has no
effect here either.

Also I suspect based on your description, that accessing *any* field of a
struct implies the whole struct must be mapped in memory. But then there are
limits in case of aliased struct like above, or even when using VLAs since you
can't figure the possible extent of the variable part.

I think I see how I could cheat to eliminate the warning (not easy to test as I
don't have the trunk version of the compiler locally), but I suspect that doing
so would definitely make the code more error-prone.

I sincerely think that this *exact* case is counter-productive as it will
basically force a number of users of lists to disable the whole warning, which
was otherwise particularly effective at detecting real out-of-bounds accesses,
especially in such use cases. Or maybe you could assign it a different level
(-Warray-bounds=3 ?) to indicate a suspicious usage that might also be a valid
one ?

Thanks!
Willy

Reply via email to