Bug 228092 – [JSC] StructureStubInfo's m_identifier should follow to the same protocol of inlineAccessBaseStructure

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] StructureStubInfo's m_identifier should follow to the same protocol of inlineAccessBaseStructure

[Yusuke Suzuki]

Created attachment 433831 [details]
Patch

[Saam Barati]

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

[Mark Lam]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r280066 (239797@main): <https://commits.webkit.org/239797@main>

[Radar WebKit Bug Importer]

<rdar://problem/80813147>