Bug 27060 – Type cast for first argument in make_pair()

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Norbert Leser]

Created attachment 32420 [details]
Code patch for Structure.cpp

Codewarrior compiler (WINSCW) latest build b482 fails with "illegal operand" when first arg of make_pair() method has unqualified type.

Proposed patch casts the arguments accordingly.

[Eric Seidel (no email)]

Comment on attachment 32420 [details]
Code patch for Structure.cpp

Why?  Also, we use c++ style casts.

[Norbert Leser]

Created attachment 34683 [details]
Updated patch for bug #27060

Symbian compiler for emulator target (WINSCW) fails with "illegal operand" for m_attributesInPrevious in structure.ccp (when calling make_pair functions). This error is apparently due to the compiler not properly resolving the unsigned type of the declared bitfield.

Initial patch explicitly casted m_attributesInPrevious to unsigned (at 3 places in structure.cpp), but since bitfield optimization is not critical for the emulator target, this conditional change in header file appears to be least intrusive.

[Darin Adler]

Comment on attachment 34683 [details]
Updated patch for bug #27060

This needs a brief comment, otherwise it's simply too mysterious.

I'm going to say review- because I don't think we should land it without a comment.

[Janne Koskinen]

>This error is apparently due to the compiler not properly resolving the
>unsigned type of the declared bitfield.

This was my assumption as well, but it doesn't seem to be true.
I made a small test program:

#include <iostream>
#include <utility>
class test{
public:
unsigned flag:1;
unsigned somefield:5;
};
using namespace std;

int main()
{
test a;
make_pair(a.flag,a.somefield);
return 1;
}

And this one passes with flying colors on WINSCW.

I'll try to narrow it down, but it does seem to be related to the curious reoccuring template pattern that is used in class defining the bitfields.

[Janne Koskinen]

Little more investigations.
This isn't about CRTP and neither is about using RefPtr. I even tried the union for accessing as it is used to traverse. It is hard to isolate structure.cpp from surroundings.

For what it's worth. I don't see casting to unsigned as a bad solution as it the argument would take 4 bytes anyways on ARM. and storing that to pair has to be 4 bytes as well.

Changing the member to be unsigned instead of 7bits is not good solution as that uses more memory and will make the struct different from what it is on other platforms.

Can't really think of anything else that might go wrong than unlikely alignment issue or bad alternative make_pair implementation.

[Janne Koskinen]

Getting there..

It is not the make_pair function that fails, but the HashMap::Contains().

in WTF/StructureTransitionTable.h we define our HashTable to be type:
    typedef HashMap<StructureTransitionTableHash::Key, Structure*, StructureTransitionTableHash, StructureTransitionTableHashTraits> StructureTransitionTable;

where the StructureTransitionTableHashTraits' SecondFirstTraits::TraitType is unsigned.

Looks like temporary pair is getting generated with unknown type. From illegal operand we can gather that this is not integer type.I looked at HashTraits.h to see if there was an obvious reason, but cold code reading doesn't help at this point. Unfortunately I don't have src for std so my debugging/preprocessing is limited here.

Again casting to unsigned is the easiest way to fix this.

[Norbert Leser]

(In reply to comment #6)
> Getting there..
> 
> It is not the make_pair function that fails, but the HashMap::Contains().
> 
> in WTF/StructureTransitionTable.h we define our HashTable to be type:
>     typedef HashMap<StructureTransitionTableHash::Key, Structure*,
> StructureTransitionTableHash, StructureTransitionTableHashTraits>
> StructureTransitionTable;
> 
> where the StructureTransitionTableHashTraits' SecondFirstTraits::TraitType is
> unsigned.
> 
> Looks like temporary pair is getting generated with unknown type. From illegal
> operand we can gather that this is not integer type.I looked at HashTraits.h to
> see if there was an obvious reason, but cold code reading doesn't help at this
> point. Unfortunately I don't have src for std so my debugging/preprocessing is
> limited here.
> 
> Again casting to unsigned is the easiest way to fix this.

If unconditional casting to unsigned is acceptable for any other platform, that would be fine with me. However, I am not certain whether that may defeat any possible 7-bit optimization.

The rationale for making the single header change to unsigned for COMPILER(WINSCW)was that this is for symbian carbide emulator only, where this space optimization is insignificant, and to avoid 3 #defines in the implementation file.

Either solution is fine with me. The question is what's the cleanest without regression on other platforms.

[Darin Adler]

Comment on attachment 34683 [details]
Updated patch for bug #27060

Seems fine. Would be better with a brief comment.

[Eric Seidel (no email)]

Comment on attachment 34683 [details]
Updated patch for bug #27060

Norbert isn't a committer, and darin is asking for a comment in the comment above, so marking this r- since no one can land this as is.

[Norbert Leser]

Created attachment 39607 [details]
2nd update of patch file for bug #27060

Same patch as before, but included comment, as suggested by Darin Adler.

[Darin Adler]

Comment on attachment 39607 [details]
2nd update of patch file for bug #27060

We can live with this change, but I am concerned about the large number of workarounds needed to work, poorly, with the WINSCW compiler.

[Simon Hausmann]

Landed in r48461