GCC miscompilation with __seg_fs

GCC miscompilation with __seg_fs

[Sergey Bugaev]

Hello,

while exploring the generated assembly for an entirely unrelated
reason, I found out that GCC eliminates stores through THREAD_SELF
when it's declared using __seg_fs. This is easily reproducible outside
of glibc; here's a sample reproducer (I've also put it on Godbolt [0]
for easy exploration):

[0]: https://godbolt.org/z/PeYv1TP8Y

#include <stddef.h>

typedef struct
{
  void *tcb;
  int some_member;
} tcbhead_t;

#define THREAD_SELF                                            \
  (*(tcbhead_t * __seg_fs *) offsetof (tcbhead_t, tcb))
#define THREAD_SELF_VOLATILE                                   \
  (*(volatile tcbhead_t * __seg_fs *) offsetof (tcbhead_t, tcb))

#define THREAD_SETMEM_SEG_FS(descr, member, value)             \
  (*(__typeof (descr->member) __seg_fs *)                      \
   offsetof (tcbhead_t, member)) = (value)

#define THREAD_SETMEM_ASM(descr, member, value)                \
  asm volatile ("movl %0, %%fs:%P1" :                          \
                : "rmi" (value),                               \
                "i" (offsetof (tcbhead_t, member)))

void
assign_using_setmem_seg_fs (void)
{
  THREAD_SETMEM_SEG_FS (THREAD_SELF, some_member, 42);
}
// assign_using_setmem_seg_fs:
//         movl $42, %fs:8
//         ret

void
assign_using_setmem_asm (void)
{
  THREAD_SETMEM_ASM (THREAD_SELF, some_member, 42);
}
// assign_using_setmem_asm:
//         movl $42, %fs:8
//         ret

void
assign_through_self (void)
{
  THREAD_SELF->some_member = 42;
}
// assign_through_self:
//         ret

void
assign_through_self_volatile (void)
{
  THREAD_SELF_VOLATILE->some_member = 42;
}
// assign_through_self_volatile:
//         movq %fs:0, %rax
//         movl $42, 8(%rax)
//         ret

void
unused_self_volatile (void)
{
    tcbhead_t *tcb = THREAD_SELF_VOLATILE;
}
// unused_self_volatile:
//         ret

As you can see, assigning to TCB members using THREAD_SETMEM
(implemented either with the inline assembly or __seg_fs) works, but
assigning them through the THREAD_SELF pointer does not. The Hurd port
does that explicitly (see __hurd_local_reply_port), and that is where
I caught the miscompilation in the act. But it is entirely possible
that NPTL suffers from this too, perhaps in code like this:

void
pthread_smth (pthread_t t)
{
  struct pthread *pt = (struct pthread *) t;
  t->some_member = 42;
}

pthread_t
__pthread_self (void)
{
  return (pthread_t) THREAD_SELF;
}

void
foo (void)
{
  pthread_smth (__pthread_self ());
}

One workaround would be redefining THREAD_SELF with volatile (see
THREAD_SELF_VOLATILE above), this makes GCC do the right thing for
assignments and does _not_ prevent optimizations if the value is
unused (see unused_self_volatile above). This does however result in
warnings:

<source>:10:30: warning: initialization discards 'volatile' qualifier
from pointer target type [-Wdiscarded-qualifiers]
   10 | #define THREAD_SELF_VOLATILE (*(volatile tcbhead_t * __seg_fs
*) offsetof (tcbhead_t, tcb))
      |                              ^
<source>:47:22: note: in expansion of macro 'THREAD_SELF_VOLATILE'
   47 |     tcbhead_t *tcb = THREAD_SELF_VOLATILE;
      |                      ^~~~~~~~~~~~~~~~~~~~

What do we do?

Sergey

Re: GCC miscompilation with __seg_fs

[Jakub Jelinek]

On Mon, Mar 13, 2023 at 06:24:47PM +0300, Sergey Bugaev wrote:
> Hello,
> 
> while exploring the generated assembly for an entirely unrelated
> reason, I found out that GCC eliminates stores through THREAD_SELF
> when it's declared using __seg_fs. This is easily reproducible outside
> of glibc; here's a sample reproducer (I've also put it on Godbolt [0]
> for easy exploration):

Smaller testcase:
typedef struct
{
  void *tcb;
  int some_member;
} tcbhead_t;

void
assign_through_self (void)
{
  (*(tcbhead_t * __seg_fs *) __builtin_offsetof (tcbhead_t, tcb))->some_member = 42;
}

From what I can see, GCC has been DSE removing these since https://gcc.gnu.org/r6-4645
when __seg_fs support has been introduced.
One can work-around it using -fno-delete-null-pointer-checks.
You could also hide the fact that it is based on 0 pointer from the
compiler...

While GCC has targetm.addr_space.zero_address_valid hook (which AFAIK only
x86 overrides), we use it only in very few spots right now.

Feel free to file a bug report in GCC bugzilla, but that won't improve
anything on the already released compilers.

	Jakub

Re: GCC miscompilation with __seg_fs

[Andrew Pinski]

On Mon, Mar 13, 2023 at 8:46 AM Jakub Jelinek via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> On Mon, Mar 13, 2023 at 06:24:47PM +0300, Sergey Bugaev wrote:
> > Hello,
> >
> > while exploring the generated assembly for an entirely unrelated
> > reason, I found out that GCC eliminates stores through THREAD_SELF
> > when it's declared using __seg_fs. This is easily reproducible outside
> > of glibc; here's a sample reproducer (I've also put it on Godbolt [0]
> > for easy exploration):
>
> Smaller testcase:
> typedef struct
> {
>   void *tcb;
>   int some_member;
> } tcbhead_t;
>
> void
> assign_through_self (void)
> {
>   (*(tcbhead_t * __seg_fs *) __builtin_offsetof (tcbhead_t, tcb))->some_member = 42;
> }
>
> From what I can see, GCC has been DSE removing these since https://gcc.gnu.org/r6-4645
> when __seg_fs support has been introduced.
> One can work-around it using -fno-delete-null-pointer-checks.
> You could also hide the fact that it is based on 0 pointer from the
> compiler...
>
> While GCC has targetm.addr_space.zero_address_valid hook (which AFAIK only
> x86 overrides), we use it only in very few spots right now.
>
> Feel free to file a bug report in GCC bugzilla, but that won't improve
> anything on the already released compilers.

I am 99% sure this is https://gcc.gnu.org/PR102733 or at least related to it.

Thanks,
Andrew Pinski

>
>         Jakub
>

Re: GCC miscompilation with __seg_fs

[Sergey Bugaev]

On Mon, Mar 13, 2023 at 6:45 PM Jakub Jelinek <jakub@redhat.com> wrote:
> You could also hide the fact that it is based on 0 pointer from the
> compiler...

But how would you do that? If we want the generated assembly to say
%fs:0, the compiler needs to statically const-propagate the pointer to
zero. If that's not important, this seems to do the trick:

static inline size_t
zero (void)
{
  size_t res;
  asm ("xor %0, %0" : "=r" (res));
  return res;
}

#define THREAD_SELF_HIDDEN                                     \
  (*(tcbhead_t * __seg_fs *) zero ())

void
assign_through_self_hidden (void)
{
  THREAD_SELF_HIDDEN->some_member = 42;
}
// assign_through_self_hidden:
//         xor %rax, %rax
//         movq    %fs:(%rax), %rax
//         movl    $42, 8(%rax)
//         ret

FWIW, without __seg_fs (where dereferencing a zero ptr is actual UB),
assigning through *(int *) 0 is enough for GCC to consider the rest of
the code dead, whereas an assignment through **(int **) 0 does not
trigger that, but the store is still eliminated:

void some_external_call (void);

void
deref_nullptr (void)
{
  *(int *) 0 = 42;
  some_external_call ();
}
// deref_nullptr:
//         movl    $0, 0
//         ud2

void
indirect_deref_nullptr (void)
{
  **(int **) 0 = 42;
  some_external_call ();
}
// indirect_deref_nullptr:
//         jmp     some_external_call

> Feel free to file a bug report in GCC bugzilla, but that won't improve
> anything on the already released compilers.

I don't even have a GCC bugzilla account, so please file it yourself.
But yes, this needs to be worked around in glibc in any case.

Sergey