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

Incorrect small struct passing in 5th argument position #127

Open
sgraham opened this issue Apr 8, 2023 · 6 comments
Open

Incorrect small struct passing in 5th argument position #127

sgraham opened this issue Apr 8, 2023 · 6 comments

Comments

@sgraham
Copy link

sgraham commented Apr 8, 2023

I found this prints "1 2 3 4 N" where N is something random (not 99).

This error doesn't occur if only 3 ints are passed before the struct, or if 5 are, so I guess it's probably an edge case in push_args(), or maybe in the setup in ND_FUNCALL.

int printf();

typedef struct StRegSized {
    unsigned char r;
} StRegSized;

void func1(int a, int b, int c, int d, StRegSized st) {
  printf("%d %d %d %d %d\n", a, b, c, d, st.r);
}

int main(void) {
  StRegSized x = {99};
  func1(1, 2, 3, 4, x);
}
@sgraham
Copy link
Author

sgraham commented Apr 8, 2023

Ah, it's actually in assign_lvar_offsets() I think. It might be that

chibicc/codegen.c

Line 1333 in 90d1f7f

if (fp + fp1 + fp2 < FP_MAX && gp + !fp1 + !fp2 < GP_MAX) {
needs to be <= not <. The other checks that are similar are doing post-increments, so they're checking before the registers-to-be-used have been added.

diff --git a/codegen.c b/codegen.c
index da11fd7..1fdff28 100644
--- a/codegen.c
+++ b/codegen.c
@@ -1330,7 +1330,7 @@ static void assign_lvar_offsets(Obj *prog) {
         if (ty->size <= 16) {
           bool fp1 = has_flonum(ty, 0, 8, 0);
           bool fp2 = has_flonum(ty, 8, 16, 8);
-          if (fp + fp1 + fp2 < FP_MAX && gp + !fp1 + !fp2 < GP_MAX) {
+          if (fp + fp1 + fp2 <= FP_MAX && gp + !fp1 + !fp2 <= GP_MAX) {
             fp = fp + fp1 + fp2;
             gp = gp + !fp1 + !fp2;
             continue;

@sgraham
Copy link
Author

sgraham commented Apr 8, 2023

Oh, but it looks like there's a second problem near this code too! In this case the c argument isn't correctly passed. Hmm.

int printf();

typedef struct StRegSized {
    unsigned long r;
} StRegSized;

void func(int a, StRegSized st, int b, StRegSized st2, int c) {
  printf("%d %d %d %d %d\n", a, st.r, b, st2.r, c);
}

int main(void) {
  StRegSized x = {99};
  StRegSized y = {98};
  func(1, x, 2, y, 3);
}

@sgraham
Copy link
Author

sgraham commented Apr 8, 2023

I think maybe assign_lvar_offsets also needs a case for structs that are <= 8 bytes, rather than assuming it should always take two registers for a struct pass.

sgraham added a commit to sgraham/dyibicc that referenced this issue Apr 8, 2023
This was incorrect (again!) for Windows. I discovered the chibicc code
at HEAD also has some problems in this area
rui314/chibicc#127.

I think Windows is correct(er) now, and made a light attempt at SysV,
but I'm not sure about SysV.

Probably needs some more tests where the one side of the caller/callee
is the host compiler (i.e. gcc/clang) to confirm that we don't just have
"two wrongs" masking an error in calling convention.
fuhsnn added a commit to fuhsnn/slimcc that referenced this issue Sep 15, 2023
@iphydf
Copy link

iphydf commented Feb 1, 2024

Another repro with 1 struct param:

#include <stdio.h>

typedef struct Wrapper {
    int wrapped;
} Wrapper;

static void broken_function(Wrapper wrapper, int param2, int param3, int param4, int param5, int param6)
{
    fprintf(stderr, "%x\n", wrapper.wrapped);
    fprintf(stderr, "%x\n", param2);
    fprintf(stderr, "%x\n", param3);
    fprintf(stderr, "%x\n", param4);
    fprintf(stderr, "%x\n", param5);
    fprintf(stderr, "%x\n", param6);  // should say 66, but says 0
}

int main(void)
{
    Wrapper wrapper = {0x11};
    int param5 = 0x55;
    int param6 = 0x66;
    broken_function(wrapper, 0x22, 0x33, 0x44, param5, param6);
}

I guess the workaround is to never pass structs.

@fuhsnn
Copy link

fuhsnn commented Feb 2, 2024

@iphydf I've been fixing ABI issues in my fork; could use some extra eyes if you are testing on the codebase.

@iphydf
Copy link

iphydf commented Feb 2, 2024

@fuhsnn TokTok/c-toxcore#2624 it works perfectly. I'm adding it to our CI to make sure it keeps working.

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

No branches or pull requests

3 participants