Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

arm: Fix floating point arguments in arm #1135

Merged
merged 1 commit into from
Mar 16, 2020

Conversation

honggyukim
Copy link
Collaborator

@honggyukim honggyukim commented Mar 5, 2020

The designated initializer in C makes implicit memset internally in arm
architecture. Due to the memset, VFP registers are clobbered
unexpectedly, so this patch fixes the problem by replacing the
designated initializer to explicit 'mcount_memset4'.

In addition, the offset of VFP registers were incorrect because the
following commit pushs d1 register on top of the original d0.

ffb69ce arm: Handle struct return type by keeping more regs on stack

So the offset is adjusted from -2 to -4 to cope with the change

Source:

  #include <stdio.h>

  float float_add(float a, float b)
  {
          fprintf(stderr, "a = %f, b = %f\n", a, b);
          return a + b;
  }

  int main(int argc, char *argv[])
  {
          double c;

          c = float_add(-0.1, 0.2);
          fprintf(stderr, "c = %f\n", c);
          return c > 0;
  }

Before:

  $ uftrace -a -F main a.out
  a = 0.000000, b = 0.000000
  c = 0.000000
  # DURATION     TID     FUNCTION
              [ 25362] | main(1, 0x7ea25344) {
              [ 25362] |   float_add(0.000000, 0.000000) {
   503.228 us [ 25362] |     fprintf(&_IO_2_1_stderr_, "a = %f, b = %f\n") = 27;
   511.197 us [ 25362] |   } = 0.000000; /* float_add */
     9.687 us [ 25362] |   fprintf(&_IO_2_1_stderr_, "c = %f\n") = 13;
   531.977 us [ 25362] | } = 0; /* main */

After:

  $ uftrace -a -F main a.out
  a = -0.100000, b = 0.200000
  c = 0.100000
  # DURATION     TID     FUNCTION
              [ 25146] | main(1, 0x7edbb344) {
              [ 25146] |   float_add(-0.100000, 0.200000) {
   501.769 us [ 25146] |     fprintf(&_IO_2_1_stderr_, "a = %f, b = %f\n") = 28;
   509.321 us [ 25146] |   } = 0.100000; /* float_add */
    12.500 us [ 25146] |   fprintf(&_IO_2_1_stderr_, "c = %f\n") = 13;
   533.539 us [ 25146] | } = 1; /* main */

Fixed: #1088

Signed-off-by: Honggyu Kim [email protected]

@honggyukim honggyukim force-pushed the check/fix-arm-float-args branch 2 times, most recently from 6647e85 to f3b678f Compare March 5, 2020 23:54
/* d0, d1 registers (64 bit) were saved below the r0 */
long *float_retval = ctx->retval - 4;

memcpy(ctx->val.v, float_retval, spec->size);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using mcount_memcpy4() here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot test this now, but changed it to mcount_memcpy4 as you suggested.

struct uftrace_trigger tr;

mcount_memset4(&tr, 0, sizeof(tr));
tr.flags = 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to reset flag anymore.. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left it to keep the original code, but removed and updated it anyway.

The designated initializer in C makes implicit memset internally in arm
architecture.  Due to the memset, VFP registers are clobbered
unexpectedly, so this patch fixes the problem by replacing the
designated initializer to explicit 'mcount_memset4'.

In addition, the offset of VFP registers were incorrect because the
following commit pushs d1 register on top of the original d0.

  ffb69ce arm: Handle struct return type by keeping more regs on stack

So the offset is adjusted from -2 to -4 to cope with the change.

Source:
  #include <stdio.h>

  float float_add(float a, float b)
  {
          fprintf(stderr, "a = %f, b = %f\n", a, b);
          return a + b;
  }

  int main(int argc, char *argv[])
  {
          double c;

          c = float_add(-0.1, 0.2);
          fprintf(stderr, "c = %f\n", c);
          return c > 0;
  }

Before:
  $ uftrace -a -F main a.out
  a = 0.000000, b = 0.000000
  c = 0.000000
  # DURATION     TID     FUNCTION
              [ 25362] | main(1, 0x7ea25344) {
              [ 25362] |   float_add(0.000000, 0.000000) {
   503.228 us [ 25362] |     fprintf(&_IO_2_1_stderr_, "a = %f, b = %f\n") = 27;
   511.197 us [ 25362] |   } = 0.000000; /* float_add */
     9.687 us [ 25362] |   fprintf(&_IO_2_1_stderr_, "c = %f\n") = 13;
   531.977 us [ 25362] | } = 0; /* main */

After:
  $ uftrace -a -F main a.out
  a = -0.100000, b = 0.200000
  c = 0.100000
  # DURATION     TID     FUNCTION
              [ 25146] | main(1, 0x7edbb344) {
              [ 25146] |   float_add(-0.100000, 0.200000) {
   501.769 us [ 25146] |     fprintf(&_IO_2_1_stderr_, "a = %f, b = %f\n") = 28;
   509.321 us [ 25146] |   } = 0.100000; /* float_add */
    12.500 us [ 25146] |   fprintf(&_IO_2_1_stderr_, "c = %f\n") = 13;
   533.539 us [ 25146] | } = 1; /* main */

Fixed: namhyung#1088

Signed-off-by: Honggyu Kim <[email protected]>
Copy link
Owner

@namhyung namhyung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@namhyung namhyung merged commit 19e6f0d into namhyung:master Mar 16, 2020
@honggyukim honggyukim deleted the check/fix-arm-float-args branch March 16, 2020 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

arm floating point argument failures
2 participants