Bug 174809 - Fix bugs in probe code to change sp on x86, x86_64 and 32-bit ARM.
Summary: Fix bugs in probe code to change sp on x86, x86_64 and 32-bit ARM.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks: 174645
  Show dependency treegraph
 
Reported: 2017-07-24 20:29 PDT by Mark Lam
Modified: 2017-07-25 13:57 PDT (History)
8 users (show)

See Also:


Attachments
proposed patch. (43.89 KB, patch)
2017-07-24 20:54 PDT, Mark Lam
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2017-07-24 20:29:51 PDT
Patch coming.
Comment 1 Radar WebKit Bug Importer 2017-07-24 20:30:41 PDT
<rdar://problem/33504759>
Comment 2 Mark Lam 2017-07-24 20:54:48 PDT
Created attachment 316347 [details]
proposed patch.
Comment 3 Build Bot 2017-07-24 20:56:38 PDT
Attachment 316347 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/testmasm.cpp:226:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/assembler/testmasm.cpp:271:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/assembler/testmasm.cpp:272:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/assembler/testmasm.cpp:318:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/assembler/testmasm.cpp:423:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/assembler/testmasm.cpp:474:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
Total errors found: 6 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Filip Pizlo 2017-07-25 13:09:11 PDT
Comment on attachment 316347 [details]
proposed patch.

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

Does testmasm run on any bots?  Maybe it should!

> Source/JavaScriptCore/assembler/testmasm.cpp:321
> +void testProbeModifiesStackPointer(std::function<void*(ProbeContext*)> computeModifiedStack)

Would WTF::Function work here?
Comment 5 Mark Lam 2017-07-25 13:57:37 PDT
Thanks for the review.

(In reply to Filip Pizlo from comment #4)
> Does testmasm run on any bots?  Maybe it should!

I'll talk to the relevant folks to get this going.

> > Source/JavaScriptCore/assembler/testmasm.cpp:321
> > +void testProbeModifiesStackPointer(std::function<void*(ProbeContext*)> computeModifiedStack)
> 
> Would WTF::Function work here?

I've changed all uses of std::function in testmasm.cpp to WTF::Function.

Landed in r219885: <http://trac.webkit.org/r219885>.