587823 - Convert MethodInfo flags into bitfield

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

The 32-bit flags field in MethodInfo is now full, and a few more bits will be necessary. Lars suggests this is a good opportunity to convert them into bitfields.

Comment 1

[Steven Johnson]

Attachment: Patch

Straightforward implementation.

Comment 2

[Lars T Hansen]

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

R- for all the naming concerns listed below; the functionality seems fine however.


Naming concerns:

_isOnlyUntypedParameters appears misnamed; _hasOnlyUntypedParameters would be better (cf _hasParamNames) or _onlyUntypedParameters; the predicate is called onlyUntypedParameters().

_isLazyRest appears misnamed; _hasLazyRest would be better, or _lazyRest; the predicate is called lazyRest().

This might be a good time to come up with a better name for _isAbstractMethod, since actual abstract methods are on their way.  (The predicate is called hasMethodBody.)

isCompiledMethod and setCompiledMethod are misnamed, isAotCompiledMethod and setAotCompiledMethods would be appropriate.

In general there does not seem to be a good correspondence between flag names and their setters/getters, even when the latter are straightforward; this is confusing.  (The flag is _needsActivation but the predicate is needActivation is just one more example, ditto _needsArguments / needArguments, _needsClosure / needClosure, _needsRest / needRest.)


Style nits:

The parens in BaseExecMgr::isVerified and BaseExecMgr::isVerifyPending are superflous.

Comment 3

[Steven Johnson]

"is" vs "has" is nitworthy IMO in the situations you mention, but no worries, I'll fix and resubmit. Agree with other concerns (esp. isAbstractMethod).

Comment 4

[Edwin Smith]

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

What's the point of, e.g.

   uint32_t                _needsArguments:1;

Since the :1 specifies the size, is "unsigned" good enough?

Comment 5

[Lars T Hansen]

Good question.

Gut feeling: "unsigned" is probably not good enough if you're hand-counting bytes in order to get alignment right - but my assumption is that if you use uint32_t the bits will be packed in uint32_t units.  That may not be true!

Comment 6

[Steven Johnson]

I'm guessing "unsigned" is fine, but in the absence of clear evidence that it's in some way superior, I'm using C99 int types...

Comment 7

[Steven Johnson]

Attachment: Patch v2

Revised with more uniform naming.

Comment 8

[Lars T Hansen]

Comment on attachment 467091 [details] [diff] [review]
Patch v2

Skimmed it this time, looks good.

Comment 9

[Steven Johnson]

pushed as http://hg.mozilla.org/tamarin-redux/rev/b6dd8a407e45

Comment 10

[Edwin Smith]

(In reply to comment #6)
> I'm guessing "unsigned" is fine, but in the absence of clear evidence that it's
> in some way superior, I'm using C99 int types...

Here's one piece of evidence in favor of "signed int" and "unsigned int" over any typedefs, specifically for bitfields:

Just happens that a bug was just fixed in nanojit related to this -- the explicit "signed" keyword had to be added.  the author defended it with some research:

https://bugzilla.mozilla.org/show_bug.cgi?id=584219#c18

Now, it could be that an explicit "uint32_t" will guarantee unsigned, but from the evidence in that comment, its not the case that an explicit "int32_t" will guarantee signed, because plain int, or a typedef, gives impl-defined behavior.

so i'm guessing in some circles, with bitfields, folks favor explicit 'signed' and 'unsigned' keywords since the int-ness and size-ness is already part of the bitfield syntax.

I certianly dont feel strongly about this.  but since the above comment just came across the wire, i figured i'd copy the reference.

Comment 11

[Lars T Hansen]

Fascinating.  No doubt it would be interesting to track down the historical baggage for that particular bit of spec language.

Would perhaps also be interesting to consider defining bit_t which is guaranteed to be an unsigned bit field:

  bit_t _lazyRest:1

The appeal would be the ability to massage the definition of bit_t to guarantee that bit fields are laid out in known-size (32-bit) chunks even on 64-bit platforms.  But maybe that's just not something we can assume to be able to do.  I'm not really advocating it - I'm happy to just do 'unsigned' everywhere and be done.

Comment 12

[Rick Reitmaier]

bits_t sounds like a good idea to me, but I'd argue that we'd want to change all bit field references in the code in one shot rather than have inconsistent usage.  Not to imply that Lars was saying otherwise, just pointing it out.