Bug 204726 - [32-bits] Check if StructureStubInfo::patch.baseTagGPR is used somewhere.
Summary: [32-bits] Check if StructureStubInfo::patch.baseTagGPR is used somewhere.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Lima
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-12-01 07:09 PST by Caio Lima
Modified: 2019-12-05 12:00 PST (History)
6 users (show)

See Also:


Attachments
Patch (5.55 KB, patch)
2019-12-01 11:46 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (19.33 KB, patch)
2019-12-03 13:42 PST, Caio Lima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Lima 2019-12-01 07:09:32 PST
If this member is never used, we can remove it and reduce sizeof(StructureStubInfo).
Comment 1 Caio Lima 2019-12-01 11:46:03 PST
Created attachment 384577 [details]
Patch
Comment 2 Mark Lam 2019-12-01 19:10:32 PST
Comment on attachment 384577 [details]
Patch

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

r=me with ChangeLog fix.

> Source/JavaScriptCore/ChangeLog:10
> +         When we generate ByIdInlineCache and ByValInlineCache, `base` is always
> +         a cell. Given that, we don't need to store `baseTag` on 32-bits
> +         version. This patch is removing StructureStubInfo::patch.baseTagGPR.

Please fix the alignment.  These lines appear to be off by one (they have 1 extra space).

Please add a comment here that the code also doesn't make use of the patch.baseTagGPR.  It's being passed around, and the register is allocated, but no instructions are being emitted that uses it.
Comment 3 Caio Lima 2019-12-03 07:11:36 PST
(In reply to Mark Lam from comment #2)
> Comment on attachment 384577 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384577&action=review
> 
> r=me with ChangeLog fix.
> 
> > Source/JavaScriptCore/ChangeLog:10
> > +         When we generate ByIdInlineCache and ByValInlineCache, `base` is always
> > +         a cell. Given that, we don't need to store `baseTag` on 32-bits
> > +         version. This patch is removing StructureStubInfo::patch.baseTagGPR.
> 
> Please fix the alignment.  These lines appear to be off by one (they have 1
> extra space).
> 
> Please add a comment here that the code also doesn't make use of the
> patch.baseTagGPR.  It's being passed around, and the register is allocated,
> but no instructions are being emitted that uses it.

Thank you very much for the review. I was thinking in the following case that needs to be checked before upstreaming this patch: We allocate baseTagGPR (it can happen on DFG) but we don't lock this into IC. If we allocate a scratch GPR to this same register, we will clobber baseTagGPR and it can cause issues if we take a slow path that is expecting an EncondedJSValue.
Comment 4 Caio Lima 2019-12-03 13:42:00 PST
Created attachment 384752 [details]
Patch
Comment 5 Caio Lima 2019-12-03 13:43:05 PST
(In reply to Caio Lima from comment #4)
> Created attachment 384752 [details]
> Patch

Checking if EWS is happy about that.
Comment 6 Caio Lima 2019-12-03 15:41:12 PST
Comment on attachment 384752 [details]
Patch

Tested internally and found some issues.
Comment 7 Caio Lima 2019-12-04 12:06:00 PST
(In reply to Caio Lima from comment #3)
>
> Thank you very much for the review. I was thinking in the following case
> that needs to be checked before upstreaming this patch: We allocate
> baseTagGPR (it can happen on DFG) but we don't lock this into IC. If we
> allocate a scratch GPR to this same register, we will clobber baseTagGPR and
> it can cause issues if we take a slow path that is expecting an
> EncondedJSValue.

I think I might be able to reproduce such case with a test. I'll upload it and a new version of patch as soon as I have it done..
Comment 8 Caio Lima 2019-12-05 12:00:53 PST
(In reply to Caio Lima from comment #3)
> 
> Thank you very much for the review. I was thinking in the following case
> that needs to be checked before upstreaming this patch: We allocate
> baseTagGPR (it can happen on DFG) but we don't lock this into IC. If we
> allocate a scratch GPR to this same register, we will clobber baseTagGPR and
> it can cause issues if we take a slow path that is expecting an
> EncondedJSValue.

I'm not confident that it is safe to upstream current patch and this will require deeper investigation. I was investigating the case where we generate IC in the `viaProxy()` on `AccessCase::generateWithGuard()`. The problem in this path is that we remove `baseTagGPR` from `usedRegisters` set and it leaves a breach to have baseTagGPR clobbered when we fallback to slow path. I'll revisit this later when having more time, since it requires more investigation.