463403 - MMgc does not expose standard VM types on all platforms

Status: VERIFIED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

Description

[Lars T Hansen]

MMgc ought to expose the same primitive types as the rest of the VM, but does not, and whether or not it does depends on the platform:

 * On Linux, at least uintptr_t is not known.
 * On Windows, at least uint8_t is not known.

This causes needless build breakage.

Comment 1

[Steven Johnson]

in my local branch I have split out all the C99 types from avmplusTypes.h into "avmplus_stdint.h" -- basically, a wraper to declare the types properly (MSVC) or just include <inttypes.h> (everywhere else). 

sounds like mmgc needs something similar... want me to clean it up and submit it as (say) "mmgc_stdint.h" ?

Comment 2

[Rick Reitmaier]

That sounds like the right solution.

Comment 3

[Steven Johnson]

I'll do something like that later today (and probably have avmplus piggyback on it rather than redeclare).

Comment 4

[Lars T Hansen]

At the risk of multiplying entities beyond necessity, should a header like this not be in a separate top-level directory (think of it as "substrate" though the name is terrible, what we're really talking about is the porting API) and included from both the VM and MMgc?  Does MMgc really need to be self-contained, and if so, to what extent does it need to be in a single directory?

Comment 5

[Steven Johnson]

long-term, I completely agree with you; both MMGC and Avmplus implement stuff that should be in a lower-level substrate. short-term, though, this is an expedient fix.

Comment 6

[Steven Johnson]

Attachment: Patch

Unify all C99 int type handling into mmgc_stdint.h (for MMgc) and avmplus_stdint.h (for avmplus). Note that the latter currently just includes the former.

Long-term, we probably should pull this sort of thing into a more well-defined base layer; this is intended as a short-term fix to ensure that all C99 int types are predictably available in MMgc, which they weren't before.

Comment 7

[Lars T Hansen]

Comment on attachment 346713 [details] [diff] [review]
Patch

It seems to me that there's a subtle change here.  With the old system one could deselect MMGC_64BIT, and intptr/uintptr would be 32-bit values, even if uintptr_t were a 64-bit value (because the compiler was doing 64-bit compiles, say).  With the new system, intptr/uintptr are always the same as intptr_t/uintptr_t, so this functionality is lost.  I'm not objecting a whole lot since it's not obvious to me that the functionality was useful or correct, but.  Have we actually lost something?

(Otherwise no objections.)

Comment 8

[Steven Johnson]

old system sounds incorrect to me -- if we were building 64-bit code that MMGC_64BIT must be defined for correct behavior. (ie what we are losing sounds like a defect in the first place.)

Comment 9

[Steven Johnson]

pushed to redux as changeset:   1061:1a44ddef4243 (plus two merge nodes)

Comment 10

[Steven Johnson]

Attachment: Patch

Update to mmgc_stdint.h that should fix compile errors on Windows Mobile.

Comment 11

[Steven Johnson]

Attachment: Patch #2

Previous patch fixed portable-build but broke visual-studio build. this should be happy for both.

Comment 12

[Brent Baker]

Comment on attachment 347608 [details] [diff] [review]
Patch #2

Verified patch works for windows mobile, windows 32bit and mac

Comment 13

[Steven Johnson]

pushed to redux as changeset:   1093:4c3337488923

Comment 14

[Chris Peyer]

Resolved fixed engineering / work item that has been pushed.  Setting status to verified.