[Bug c/96040] New: Compiled code causes SIGBUS at -O2

[Bug c/96040] New: Compiled code causes SIGBUS at -O2

[josephcsible at gmail dot com]

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

            Bug ID: 96040
           Summary: Compiled code causes SIGBUS at -O2
           Product: gcc
           Version: 10.1.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: josephcsible at gmail dot com
  Target Milestone: ---
            Target: x86_64-linux-gnu

Consider this C code:

int puts(const char *);
int snprintf(char *, unsigned long, const char *, ...);
unsigned long strspn(const char *, const char *);

struct TValue {
  union {
    long long i;
    double n;
  } value_;
  unsigned char tt_;
};

static int tostringbuff (struct TValue *num, char *str) {
  int len;
  if (num->tt_ == 3) {
    len = snprintf(str,50,"%lld",num->value_.i);
  } else {
    len = snprintf(str,50,"%.14g",num->value_.n);
    if (str[strspn(str, "-0123456789")] == '\0') {
      str[len++] = '.';
      str[len++] = '0';
    }
  }
  return len;
}

void unused (int *buff, struct TValue *num) {
  char junk[50];
  *buff += tostringbuff(num, junk);
}

char space[400];

void addnum2buff (int *buff, struct TValue *num) __attribute__((__noinline__));
void addnum2buff (int *buff, struct TValue *num) {
  *buff += tostringbuff(num, space);
}

int main(void) {
    int buff = 0;
    struct TValue num;
    num.value_.n = 1.0;
    num.tt_ = 19;
    addnum2buff(&buff, &num);
    puts(space);
}

It's supposed to print "1.0". When compiled with "gcc -O2", it instead crashes
with SIGBUS. This appears to be a regression, since it works fine on GCC 9.

The minimization is my own, but the bug was originally found in the wild by
actboy168 compiling Lua 5.4.0 on Arch Linux:
http://lua-users.org/lists/lua-l/2020-07/msg00001.html

https://godbolt.org/z/RMc3RX

[Bug ipa/96040] [10/11 Regression] Compiled code causes SIGBUS at -O2

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
            Summary|Compiled code causes SIGBUS |[10/11 Regression] Compiled
                   |at -O2                      |code causes SIGBUS at -O2
                 CC|                            |jakub at gcc dot gnu.org,
                   |                            |marxin at gcc dot gnu.org
          Component|c                           |ipa
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-07-02
   Target Milestone|---                         |10.2

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Started with r10-4651-gafeb887562af17ea235fbec650ff6d16c412682a
>From quick look it seems like a disagreement on how many arguments
tostringbuff.part.0.isra has.

[Bug ipa/96040] [10/11 Regression] Compiled code causes SIGBUS at -O2

[josephcsible at gmail dot com]

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

--- Comment #2 from Joseph C. Sible <josephcsible at gmail dot com> ---
Andrew Gierth posted this to the Lua mailing list:

> I think I see what's happening here, but I don't think I have an account
> on the gcc bug tracker to post it there (feel free to forward this).
> It's not (I think) a confusion over how many arguments the specialized
> tostringbuff.part.0.isra function has, but rather over the type, or
> equivalently the register allocation, for the first parameter.
> 
> The callsite is putting num->value_.n into %rdi, and &space into %rsi,
> as if the function were declared as taking (long long v, char *buf)
> rather than (double v, char *buf) which would require that num->value_.n
> be placed in %xmm0 and &space into %rdi.
> 
> But the code of the function itself is assuming that the incoming values
> are in %xmm0 and %rdi - %xmm0 is not touched because it's already in the
> right place for the snprintf call, while %rdi is left as the first arg
> to snprintf. The value 0x3ff0000000000000 is of course 1.0 as a double,
> which naturally does not work well as a pointer so it blows up.

I agree with the gist of that. I think this is more of a calling convention
issue than an argument count issue. It's as if the following pseudo-C were
written:

static int tostringbuff.part.0.isra.0 (union { long long i; double n; } value_
/* rdi */, char *str /* rsi */);

void addnum2buff (int *buff, struct TValue *num) {
    if(num->tt_ == 3) {
        buff += snprintf(space,50,"%lld",num->value_.i);
    } else {
        buff += tostringbuff.part.0.isra.0(num->value_ /* rdi */, space /* rsi
*/);
    }
}

static int tostringbuff.part.0.isra.0 (double n /* xmm0 */, char *str /* rdi
*/) {
    int len;
    len = snprintf(str,50,"%.14g",n);
    if(str[strspn(str, "-0123456789")] == '\0') {
      str[len++] = '.';
      str[len++] = '0';
    }
    return len;
}

In English, the compiler couldn't make up its mind as to how
tostringbuff.part.0.isra.0 was declared. At the call site, the first parameter
was a union, but at the definition, the first parameter was a double. Mixing up
a union and its element is fine when they only live in memory since they have
the same address, but here it was in a register. Since the calling convention
puts those types in different kinds of registers, what the function thought was
a pointer was actually the double value, predictably causing a crash when it
was dereferenced.

[Bug ipa/96040] [10/11 Regression] Compiled code causes SIGBUS at -O2

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
                 CC|                            |jamborm at gcc dot gnu.org

[Bug ipa/96040] [10/11 Regression] Compiled code causes SIGBUS at -O2

[rguenth at gcc dot gnu.org]

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

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
Indeed in the IL I see long long actual arguments passed but the function
argument type is double.  It looks like somehow argument modification
of the calls fails or is incomplete.  Martin?

[Bug ipa/96040] [10/11 Regression] Compiled code causes SIGBUS at -O2

[jamborm at gcc dot gnu.org]

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

Martin Jambor <jamborm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jamborm at gcc dot gnu.org

--- Comment #4 from Martin Jambor <jamborm at gcc dot gnu.org> ---
I'll have a look

[Bug ipa/96040] [10/11 Regression] Compiled code causes SIGBUS at -O2

[jamborm at gcc dot gnu.org]

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

--- Comment #5 from Martin Jambor <jamborm at gcc dot gnu.org> ---
IPA-split puts the double access to the union in the .part function
and keeps only the long int access in the "original" function.
IPA-SRA thinks it can work with that but the code in "transitive" call
parameter splitting apparently does not handle this case properly.

The easiest fix and probably the one most suitable for backporting is
to prevent splitting of such unions with the following:

--- a/gcc/ipa-sra.c
+++ b/gcc/ipa-sra.c
@@ -3271,7 +3271,9 @@ all_callee_accesses_present_p (isra_param_desc
*param_desc,
        continue;
       param_access *pacc = find_param_access (param_desc, argacc->unit_offset,
                                              argacc->unit_size);
-      if (!pacc || !pacc->certain)
+      if (!pacc
+         || !pacc->certain
+         || !types_compatible_p (argacc->type, pacc->type))
        return false;
     }
   return true;


Alternatively, we can of course handle the type mismatch and insert
appropriate V_C_E:

diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
index 2cc4bc79dc1..de9bad78712 100644
--- a/gcc/ipa-param-manipulation.c
+++ b/gcc/ipa-param-manipulation.c
@@ -641,6 +641,12 @@ ipa_param_adjustments::modify_call (gcall *stmt,
            && trans_map[j].unit_offset == apm->unit_offset)
          {
            repl = trans_map[j].repl;
+           if (!useless_type_conversion_p (apm->type, TREE_TYPE (repl)))
+             {
+               repl = build1 (VIEW_CONVERT_EXPR, apm->type, repl);
+               repl = force_gimple_operand_gsi (&gsi, repl, true, NULL, true,
+                                                GSI_SAME_STMT);
+             }
            break;
          }
       if (repl)

[Bug ipa/96040] [10/11 Regression] Compiled code causes SIGBUS at -O2

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
V_C_Es could be only used if both types have the same size, is that checked
somewhere that the union e.g. doesn't have int and double members instead?

[Bug ipa/96040] [10/11 Regression] Compiled code causes SIGBUS at -O2

[jamborm at gcc dot gnu.org]

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

--- Comment #7 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Yes, IPA-SRA identifies accesses by both offset and size, so the situation
would not have happened if the size was different.

[Bug ipa/96040] [10/11 Regression] Compiled code causes SIGBUS at -O2

[jakub at gcc dot gnu.org]

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Ok.  Still, there can be problems in holding non-floating values in floating
point parameters, because not all bits are necessarily preserved (e.g. padding
bits inside of the floating point format).

[Bug ipa/96040] [10/11 Regression] Compiled code causes SIGBUS at -O2

[jamborm at gcc dot gnu.org]

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

--- Comment #9 from Martin Jambor <jamborm at gcc dot gnu.org> ---
True. Richi expressed preference for avoiding the transform when there are type
mismatches, so I'm currently bootstrapping that.  I guess we can always revisit
the decision if we ever discover it would be really beneficial to perform the
split.

[Bug ipa/96040] [10/11 Regression] Compiled code causes SIGBUS at -O2

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Martin Jambor <jamborm@gcc.gnu.org>:

https://gcc.gnu.org/g:b9a15a8325ba89b926e3c437b7961829a6b2fa2b

commit r11-1809-gb9a15a8325ba89b926e3c437b7961829a6b2fa2b
Author: Martin Jambor <mjambor@suse.cz>
Date:   Fri Jul 3 17:37:33 2020 +0200

    ipa-sra: Avoid transitive splits with type mismatches (PR 96040)

    PR 96040 revealed IPA-SRA, when checking whether an intended split is
    the same as the one in a called function does not also check if the
    types match and the transformation code does not handle any resulting
    type mismatches.  This patch simply avoids the the split in the case
    of mismatches, so that we do not have to be careful about invalid
    floating-point values being passed in floating point registers and
    related issues.

    gcc/ChangeLog:

    2020-07-03  Martin Jambor  <mjambor@suse.cz>

            PR ipa/96040
            * ipa-sra.c (all_callee_accesses_present_p): Do not accept type
            mismatched accesses.

    gcc/testsuite/ChangeLog:

    2020-07-03  Martin Jambor  <mjambor@suse.cz>

            PR ipa/96040
            * gcc.dg/ipa/pr96040.c: New test.

[Bug ipa/96040] [10/11 Regression] Compiled code causes SIGBUS at -O2

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Martin Jambor
<jamborm@gcc.gnu.org>:

https://gcc.gnu.org/g:56a34e3e1cbb7d3b2f9298c14d4d3a3a030c7755

commit r10-8425-g56a34e3e1cbb7d3b2f9298c14d4d3a3a030c7755
Author: Martin Jambor <mjambor@suse.cz>
Date:   Sat Jul 4 19:46:52 2020 +0200

    ipa-sra: Avoid transitive splits with type mismatches (PR 96040)

    PR 96040 revealed IPA-SRA, when checking whether an intended split is
    the same as the one in a called function does not also check if the
    types match and the transformation code does not handle any resulting
    type mismatches.  This patch simply avoids the the split in the case
    of mismatches, so that we do not have to be careful about invalid
    floating-point values being passed in floating point registers and
    related issues.

    gcc/ChangeLog:

    2020-07-03  Martin Jambor  <mjambor@suse.cz>

            PR ipa/96040
            * ipa-sra.c (all_callee_accesses_present_p): Do not accept type
            mismatched accesses.

    gcc/testsuite/ChangeLog:

    2020-07-03  Martin Jambor  <mjambor@suse.cz>

            PR ipa/96040
            * gcc.dg/ipa/pr96040.c: New test.

    (cherry picked from commit b9a15a8325ba89b926e3c437b7961829a6b2fa2b)

[Bug ipa/96040] [10/11 Regression] Compiled code causes SIGBUS at -O2

[jamborm at gcc dot gnu.org]

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

Martin Jambor <jamborm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #12 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Fixed.