Bug 228092 - [JSC] StructureStubInfo's m_identifier should follow to the same protocol of inlineAccessBaseStructure
Summary: [JSC] StructureStubInfo's m_identifier should follow to the same protocol of ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-19 15:59 PDT by Yusuke Suzuki
Modified: 2021-07-19 18:31 PDT (History)
7 users (show)

See Also:


Attachments
Patch (14.93 KB, patch)
2021-07-19 16:02 PDT, Yusuke Suzuki
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2021-07-19 15:59:19 PDT
[JSC] StructureStubInfo's m_identifier should follow to the same protocol of inlineAccessBaseStructure
Comment 1 Yusuke Suzuki 2021-07-19 16:02:08 PDT
Created attachment 433831 [details]
Patch
Comment 2 Saam Barati 2021-07-19 16:13:56 PDT
Comment on attachment 433831 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:10
> +        m_identifier field too. This patch makes m_identifier follow to the same protocol of inlineAccessBaseStructure
> +        so that we fix this race issue too.

I'd document just for completeness what we're doing specifically (so folks don't have to read r279813 change)

> Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:-422
> -void StructureStubInfo::setCacheType(const ConcurrentJSLockerBase&, CacheType newCacheType)

I would keep this function. It's nice to document/enforce that we're holding the lock. See https://trac.webkit.org/changeset/264388/webkit
Comment 3 Mark Lam 2021-07-19 16:22:14 PDT
Comment on attachment 433831 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:-422
>> -void StructureStubInfo::setCacheType(const ConcurrentJSLockerBase&, CacheType newCacheType)
> 
> I would keep this function. It's nice to document/enforce that we're holding the lock. See https://trac.webkit.org/changeset/264388/webkit

Maybe make it inline since it's no-op function now?
Comment 4 Yusuke Suzuki 2021-07-19 16:26:59 PDT
Comment on attachment 433831 [details]
Patch

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

>>> Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:-422
>>> -void StructureStubInfo::setCacheType(const ConcurrentJSLockerBase&, CacheType newCacheType)
>> 
>> I would keep this function. It's nice to document/enforce that we're holding the lock. See https://trac.webkit.org/changeset/264388/webkit
> 
> Maybe make it inline since it's no-op function now?

For now, I'll just make it

ALWAYS_INLINE void StructureStubInfo::setCacheType(const ConcurrentJSLockerBase&, CacheType newCacheType)
{
    m_cacheType = newCacheType;
}

But personally, thread-safety-annotation would be better to me in this case since we already get Locker parameter in each function which modifies this field. (but currently, not sure how to annotate it via that if it depends on CodeBlock's lock.
Comment 5 Yusuke Suzuki 2021-07-19 18:30:07 PDT
Committed r280066 (239797@main): <https://commits.webkit.org/239797@main>
Comment 6 Radar WebKit Bug Importer 2021-07-19 18:31:17 PDT
<rdar://problem/80813147>