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

All bits in the union returned by make_temp_reg should be initialised #1551

Merged
merged 1 commit into from
Sep 24, 2021

Conversation

nwc10
Copy link
Contributor

@nwc10 nwc10 commented Sep 24, 2021

make_temp_reg returns MVMSpeshOperand, which is a union between various
types and structs, all of which are 64 bits or smaller.

Code in optimize_bb_switch evaluates lit_i64 != -1 before calling
MVM_spesh_manipulate_release_temp_reg. The struct within the union returned
from make_temp_reg is only 48 bits, so take care to ensure that the other
16 bits are initialised, else valgrind (correctly) complains that
"Conditional jump or move depends on uninitialised value(s)".

(To be fair, technically the subsequent code shouldn't be reading the union
using a different member than it was written with. If compilers are going to
get exacting about this - ie infer that writing the lit_i64 member can be
completely eliminated - then we will need to change the struct within the
union to be exactly 64 bits in total.)

`make_temp_reg` returns `MVMSpeshOperand`, which is a union between various
types and structs, all of which are 64 bits or smaller.

Code in `optimize_bb_switch` evaluates `lit_i64 != -1` before calling
`MVM_spesh_manipulate_release_temp_reg`. The struct within the union returned
from `make_temp_reg` is only 48 bits, so take care to ensure that the other
16 bits are initialised, else valgrind (correctly) complains that
"Conditional jump or move depends on uninitialised value(s)".

(To be fair, technically the subsequent code shouldn't be reading the union
using a different member than it was written with. If compilers are going to
get exacting about this - ie infer that writing the `lit_i64` member can be
completely eliminated - then we will need to change the struct within the
union to be exactly 64 bits in total.)
@nwc10
Copy link
Contributor Author

nwc10 commented Sep 24, 2021

@jnthn - in looking at this code

    /* Allocate temporary and set up result. */
    g->temps[g->num_temps].orig   = result.reg.orig = g->num_locals;
    g->temps[g->num_temps].i      = result.reg.i    = 0;
    g->temps[g->num_temps].used_i = 0;
    g->temps[g->num_temps].kind   = kind;
    g->temps[g->num_temps].in_use = 1;

The struct being assigned to is this?

/* A temporary register, added to support transformations. */
struct MVMSpeshTemporary {
    /* The number of the local along with the current SSA index. */
    MVMuint16 orig;
    MVMuint16 i;

    /* The SSA index currently loaned out. */
    MVMuint16 used_i;

    /* What kind of register is it? */
    MVMuint16 kind;

    /* Is it currently in use? */
    MVMuint16 in_use;
};

In which case, I was a bit troubled that its i is MVMuint16, whereas that of union MVMSpeshOperand is MVMint32. Here both are getting set to 0, which won't overflow :-) But really shouldn't the codebase be consistent on what size the type should be?

@jnthn
Copy link
Member

jnthn commented Sep 24, 2021

@nwc10 If you'd asked me what the type of i in MVMSpeshOperand was without looking, I'd have said MVMuint16! Yes, consistency is good.

@jnthn
Copy link
Member

jnthn commented Sep 24, 2021

Ah, and on overflow risk: the upper limit on bytecode size to specialize is 65536 bytes. You can't create code in that space that will have more than 65536 versions!

@jnthn jnthn merged commit b9cb40f into new-disp Sep 24, 2021
@jnthn jnthn deleted the make_temp_reg-init-all-64 branch September 24, 2021 10:55
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.

2 participants