Closed Bug 450765 Opened 14 years ago Closed 14 years ago

Fix type-punned warnings in LIR.h


(Tamarin Graveyard :: Tracing Virtual Machine, defect)

Not set


(Not tracked)



(Reporter: mrbkap, Assigned: mrbkap)




(1 file)

Attached patch FixSplinter Review
When compiling nanojit, I see lots of:

nanojit/LIR.h: In member function ‘uint64_t nanojit::LIns::constvalq() const’:
nanojit/LIR.h:281: warning: dereferencing type-punned pointer will break strict-aliasing rules
nanojit/LIR.h: In member function ‘double nanojit::LIns::constvalf() const’:
nanojit/LIR.h:305: warning: dereferencing type-punned pointer will break strict-aliasing rules

AFAICT, the warnings are real and easy to fix.
Attachment #333960 - Flags: review?(edwsmith)
Note: due to time pressure, I've checked this patch into the tracemonkey tree as
That change probably needs to be endian-specific.
Only the second hunk needs to be, right? The first bit is basically performing a memcpy.
depends on how the values are inserted into the LIR stream in the first place -- if we're always pulling them out in the same order as we put them in, it probably doesn't matter... but it's probably good form to always store them in platform order
Wait, I lost it. There's no endian difference between the new code and old code here. The cast and union have exactly the same semantics, except the union tells the compiler which names refer to which memory so it doesn't over optimize. If the new code needs to be endian specific, so does the old code.
True. Maybe it didn't matter before because we were always shoving them in, and pulling them out, in the same order? Or maybe we just hadn't tested on a big-endian machine? 
I don't know what this code has been tested on, but it's true that it didn't matter before because you're just shoving them in and pulling them out. I don't know enough of nanojit's internals to know how we get here in the first place.
Attachment #333960 - Flags: review?(edwsmith) → review+
Comment on attachment 333960 [details] [diff] [review]

Contact if you need someone to push this once/if it's updated to TT tip.  thanks.
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 793494
You need to log in before you can comment on or make changes to this bug.