Bug 232019 - We should watch isHavingABadTime if we read from the structureCache
Summary: We should watch isHavingABadTime if we read from the structureCache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Michaud
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-20 09:30 PDT by Justin Michaud
Modified: 2021-10-22 11:19 PDT (History)
6 users (show)

See Also:

Attachments
Patch (7.44 KB, patch)
2021-10-20 09:45 PDT, Justin Michaud 		no flags Details | Formatted Diff | Diff
Patch (7.64 KB, patch)
2021-10-20 14:08 PDT, Justin Michaud 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Michaud 2021-10-20 09:30:44 PDT 
rdar://84338462

We should watch isHavingABadTime if we read from the structureCache off the mutator thread
Comment 1 Justin Michaud 2021-10-20 09:45:46 PDT 
Created attachment 441889 [details]
Patch
Comment 2 Yusuke Suzuki 2021-10-20 11:19:47 PDT 
Comment on attachment 441889 [details]
Patch

r=me
Comment 3 Yusuke Suzuki 2021-10-20 11:44:49 PDT 
Comment on attachment 441889 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3131
> +                if (!globalObject->isHavingABadTime())

I think you need to insert load-load-fence just after this. So,

bool isHavingABadTime = globalObject->isHavingABadTime();
WTF::loadLoadFence();
if (!isHavingABadTime)
    m_graph.watchpoints().addLazily(globalObject->havingABadTimeWatchpoint());

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:846
> +                        if (!globalObject->isHavingABadTime())

Ditto
Comment 4 Justin Michaud 2021-10-20 14:08:50 PDT 
Created attachment 441931 [details]
Patch
Comment 5 EWS 2021-10-20 15:08:52 PDT 
Committed r284576 (243316@main): <https://commits.webkit.org/243316@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441931 [details].
Comment 6 Saam Barati 2021-10-22 11:19:00 PDT 
Comment on attachment 441931 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3138
> +                WTF::loadLoadFence();

Why do we have 2 load load fences?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3140
> +                structure = m_vm.structureCache
> +                    .emptyObjectStructureConcurrently(globalObject, base.getObject(), JSFinalObject::defaultInlineCapacity());

this should not be on multiple lines.

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:855
> +                        structure = globalObject->vm().structureCache
> +                            .emptyObjectStructureConcurrently(globalObject, base.getObject(), JSFinalObject::defaultInlineCapacity());

ditto, not 2 lines.
If you really like putting things on 2 lines, you should do it for the arguments, not the "."