[Bug tree-optimization/64703] New: glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile

[Bug tree-optimization/64703] New: glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile

[amodra at gmail dot com]

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

            Bug ID: 64703
           Summary: glibc sysdeps/powerpc/powerpc64/dl-machine.h
                    miscompile
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: amodra at gmail dot com

This testcase, extracted from glibc sysdeps/powerpc/powerpc64/dl-machine.h
shows gcc optimising away the "opd" initialisation at -O1.  ie. it's as if the
indirect call doesn't cause gcc to see that "value" is used.  Adding the asm
cures the problem, as does passing the function pointer to another function
rather than calling via function pointer in resolve_ifunc.

struct link_map {
  unsigned long l_addr;
  int l_relocated;
};

typedef struct {
  unsigned long fd_func;
  unsigned long fd_toc;
  unsigned long fd_aux;
} Elf64_FuncDesc;

extern const struct link_map *dl_rtld_map;
extern unsigned long dl_hwcap;

unsigned long
resolve_ifunc (unsigned long value,
           const struct link_map *map, const struct link_map *sym_map)
{
  Elf64_FuncDesc opd;

  if (map != sym_map
      && sym_map != dl_rtld_map
      && !sym_map->l_relocated)
    {
      Elf64_FuncDesc *func = (Elf64_FuncDesc *) value;
      opd.fd_func = func->fd_func + sym_map->l_addr;
      opd.fd_toc = func->fd_toc + sym_map->l_addr;
      opd.fd_aux = func->fd_aux;
      value = (unsigned long) &opd;
    }
#if 0
  __asm__ ("#%0" : : "r" (value));
#endif
  return ((unsigned long (*) (unsigned long)) value) (dl_hwcap);
}

[Bug tree-optimization/64703] glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Even though I think there is a bug here.  I think the best way to work around
this issue is do:
unsigned long
resolve_ifunc (unsigned long value,
           const struct link_map *map, const struct link_map *sym_map)
{
  Elf64_FuncDesc opd;

  if (map != sym_map
      && sym_map != dl_rtld_map
      && !sym_map->l_relocated)
    {
      Elf64_FuncDesc *func = (Elf64_FuncDesc *) value;
      opd.fd_func = func->fd_func + sym_map->l_addr;
      opd.fd_toc = func->fd_toc + sym_map->l_addr;
      opd.fd_aux = func->fd_aux;
      value = (unsigned long) &opd;
    }
  asm("":"+r"(value): : "memory");
  return ((unsigned long (*) (unsigned long)) value) (dl_hwcap);
}

[Bug tree-optimization/64703] glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
4.6 seems to optimize it away too, 4.4 does not.  Anyway, strictly speaking the
code is invalid, the standard does not allow conversions between data and
function pointers I believe.  I'd say the asm barrier in the code would be
best.

[Bug target/64703] glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Target|                            |powerpc64
          Component|tree-optimization           |target

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
The tree level gets this right and on x86_64 RTL expansion looks ok as well.
Can it be that PPC relies on REG_POINTER stuff and maybe the function
pointer isn't marked that way?

That said, looks like a RTL / backend issue to me.

[Bug target/64703] glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #3)
> The tree level gets this right and on x86_64 RTL expansion looks ok as well.
> Can it be that PPC relies on REG_POINTER stuff and maybe the function
> pointer isn't marked that way?
> 
> That said, looks like a RTL / backend issue to me.

The opd stores were removed by tree dse:
--- pr64703.c.090t.phicprop1    2015-01-21 10:41:50.673199149 +0100
+++ pr64703.c.091t.dse1    2015-01-21 10:41:50.674199132 +0100
@@ -42,12 +42,9 @@ resolve_ifunc (long unsigned int value,
   _10 = func_9->fd_func;
   _11 = sym_map_4(D)->l_addr;
   _12 = _10 + _11;
-  opd.fd_func = _12;
   _14 = func_9->fd_toc;
   _15 = _11 + _14;
-  opd.fd_toc = _15;
   _17 = func_9->fd_aux;
-  opd.fd_aux = _17;
   value_19 = (long unsigned int) &opd;

   <bb 6>:
so I don't see how this would be a RTL or backend problem.  Though, of course
the backend would need to somehow explain to the aliasing machinery that it has
something like function descriptors and what they really mean for function
calls.  Unless we want to disable DSE altogether before any function calls,
eek.

[Bug target/64703] glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile

[schwab@linux-m68k.org]

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

--- Comment #5 from Andreas Schwab <schwab@linux-m68k.org> ---
A conversion between an integer type and a pointer type is
implementation-defined, not undefined.

[Bug target/64703] glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile

[amodra at gmail dot com]

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

--- Comment #6 from Alan Modra <amodra at gmail dot com> ---
Re comment #2, yes I agree that conversion between void* or pointer to object
and pointer to function is not strictly allowed.  Fixing that by way of a union
was one of the first things I tried, before looking at dumps and finding that
tree dse was where we lost the opd stores.  And yes, the testcase can be
compiled on x86_64 where (unsurprisingly) the stores are seen as dead too.

[Bug target/64703] glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile

[amodra at gmail dot com]

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

--- Comment #7 from Alan Modra <amodra at gmail dot com> ---
Created attachment 34539
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=34539&action=edit
prototype patch

How does this look as a potential fix?  Obviously I'd need to provide the new
targetm field and arrange for it to be set on powerpc64 elfv1, but before I do
that I'd like some indication that this is going in the right direction.

[Bug target/64703] glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |msebor at gcc dot gnu.org

--- Comment #8 from Martin Sebor <msebor at gcc dot gnu.org> ---
The test case is invalid because it accesses the value of the opd object via a
pointer to a function when C allows an object to have its value accessed only
by an lvalue of a compatible type.

A simpler test case that demonstrates a similar issue is below:

#include <assert.h>

struct X { const char *s; };
struct Y { unsigned long n; };

const char* __attribute__ ((weak))
foo (unsigned long value) {
    struct Y y;

    if (value) {
        y.n = ((struct Y*)value)->n;
        value = (unsigned long)&y;
    }

    return ((struct X*)value)->s;
}

int main (void) {
    struct Y y = { 123 }; 

    unsigned long n = (unsigned long)foo ((unsigned long)&y);

    assert (n == y.n);

    return 0;
}

[Bug target/64703] glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile

[amodra at gmail dot com]

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

Alan Modra <amodra at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |WONTFIX

--- Comment #9 from Alan Modra <amodra at gmail dot com> ---
I may as well close this in view of the fact that fixing the problem very
likely will result in losing optimizations.