Bug 175994

Summary: Define *_GIGACAGE_MASK when Gigacage is not supported
Product: WebKit Reporter: Don Olmstead <don.olmstead>
Component: JavaScriptCoreAssignee: Don Olmstead <don.olmstead>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, cdumez, cmarcelo, commit-queue, dbates, fpizlo, mark.lam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Don Olmstead 2017-08-25 13:37:45 PDT
After https://trac.webkit.org/changeset/221148/webkit the WinCairo bots report the following errors

error C2065: 'JSVALUE_GIGACAGE_MASK': undeclared identifier
error C2065: 'PRIMITIVE_GIGACAGE_MASK': undeclared identifier
Comment 1 Don Olmstead 2017-08-25 13:41:28 PDT
Created attachment 319095 [details]
Patch

Fix suggested by mlam on IRC. Built WinCairo successfully.
Comment 2 Filip Pizlo 2017-08-25 13:42:25 PDT
Comment on attachment 319095 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319095&action=review

> Source/WTF/wtf/Gigacage.h:33
> +#define PRIMITIVE_GIGACAGE_MASK UINTPTR_MAX
> +#define JSVALUE_GIGACAGE_MASK UINTPTR_MAX

Why?
Comment 3 Mark Lam 2017-08-25 13:44:29 PDT
Comment on attachment 319095 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319095&action=review

>> Source/WTF/wtf/Gigacage.h:33
>> +#define JSVALUE_GIGACAGE_MASK UINTPTR_MAX
> 
> Why?

The wincairo build which does not use bmalloc is failing to build.  Is this the wrong fix for when bmalloc is not used?
Comment 4 Filip Pizlo 2017-08-25 13:45:05 PDT
(In reply to Mark Lam from comment #3)
> Comment on attachment 319095 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319095&action=review
> 
> >> Source/WTF/wtf/Gigacage.h:33
> >> +#define JSVALUE_GIGACAGE_MASK UINTPTR_MAX
> > 
> > Why?
> 
> The wincairo build which does not use bmalloc is failing to build.  Is this
> the wrong fix for when bmalloc is not used?

I see.  OK.
Comment 5 Filip Pizlo 2017-08-25 13:45:18 PDT
Comment on attachment 319095 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319095&action=review

>>>> Source/WTF/wtf/Gigacage.h:33
>>>> +#define JSVALUE_GIGACAGE_MASK UINTPTR_MAX
>>> 
>>> Why?
>> 
>> The wincairo build which does not use bmalloc is failing to build.  Is this the wrong fix for when bmalloc is not used?
> 
> I see.  OK.

Please set these to 0, not UINTPTR_MAX.
Comment 6 Mark Lam 2017-08-25 13:49:00 PDT
Comment on attachment 319095 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319095&action=review

>>>>> Source/WTF/wtf/Gigacage.h:33
>>>>> +#define JSVALUE_GIGACAGE_MASK UINTPTR_MAX
>>>> 
>>>> Why?
>>> 
>>> The wincairo build which does not use bmalloc is failing to build.  Is this the wrong fix for when bmalloc is not used?
>> 
>> I see.  OK.
> 
> Please set these to 0, not UINTPTR_MAX.

At first, I thought that the llint code is unconditionally applying the masm.  But I see that the llint code only applies the mask if GIGACAGE_ENABLED.  Hence, setting these to 0 makes sense.
Comment 7 Don Olmstead 2017-08-25 13:50:24 PDT
Created attachment 319096 [details]
Patch

Setting masks to 0
Comment 8 Mark Lam 2017-08-25 13:56:36 PDT
Comment on attachment 319096 [details]
Patch

r=me if EWS is green and if this patch makes the code build on wincairo (which was where this issue was detected) and tests passes there as well.
Comment 9 WebKit Commit Bot 2017-08-25 16:11:28 PDT
Comment on attachment 319096 [details]
Patch

Clearing flags on attachment: 319096

Committed r221211: <http://trac.webkit.org/changeset/221211>
Comment 10 WebKit Commit Bot 2017-08-25 16:11:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2017-08-25 16:12:36 PDT
<rdar://problem/34091627>