MMgc does not expose standard VM types on all platforms

VERIFIED FIXED

Status

Tamarin
Garbage Collection (mmGC)
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Lars T Hansen, Assigned: Steven Johnson)

Tracking

Details

Attachments

(2 attachments, 1 obsolete attachment)

9.45 KB, patch
Lars T Hansen
: review+
Details | Diff | Splinter Review
959 bytes, patch
Brent Baker
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
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

10 years ago
That sounds like the right solution.
(Assignee)

Comment 3

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

Comment 4

10 years ago
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?
(Assignee)

Comment 5

10 years ago
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.
(Assignee)

Comment 6

10 years ago
Created attachment 346713 [details] [diff] [review]
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.
Assignee: nobody → stejohns
Attachment #346713 - Flags: review?(lhansen)
(Reporter)

Comment 7

10 years ago
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.)
Attachment #346713 - Flags: review?(lhansen) → review+
(Assignee)

Comment 8

10 years ago
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.)
(Assignee)

Comment 9

10 years ago
pushed to redux as changeset:   1061:1a44ddef4243 (plus two merge nodes)
(Assignee)

Comment 10

10 years ago
Created attachment 347606 [details] [diff] [review]
Patch

Update to mmgc_stdint.h that should fix compile errors on Windows Mobile.
Attachment #347606 - Flags: review?(brbaker)
(Assignee)

Comment 11

10 years ago
Created attachment 347608 [details] [diff] [review]
Patch #2

Previous patch fixed portable-build but broke visual-studio build. this should be happy for both.
Attachment #347606 - Attachment is obsolete: true
Attachment #347608 - Flags: review?(brbaker)
Attachment #347606 - Flags: review?(brbaker)

Updated

10 years ago
Attachment #347608 - Flags: review?(brbaker) → review+

Comment 12

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

Verified patch works for windows mobile, windows 32bit and mac
(Assignee)

Comment 13

10 years ago
pushed to redux as changeset:   1093:4c3337488923
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 14

9 years ago
Resolved fixed engineering / work item that has been pushed.  Setting status to verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.