Bug 150773 - Rename op_put_getter_setter to op_put_getter_setter_by_id
Summary: Rename op_put_getter_setter to op_put_getter_setter_by_id
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:
Depends on: 150780
Blocks:
  Show dependency tree / graph
 
Reported: 2015-11-01 06:03 PST by Yusuke Suzuki
Modified: 2015-11-02 11:12 PST (History)
8 users (show)

See Also:

Attachments
Patch (12.22 KB, patch)
2015-11-01 06:04 PST, Yusuke Suzuki 		mark.lam: review+
Details | Formatted Diff | Diff
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 Yusuke Suzuki 2015-11-01 06:03:22 PST 
Rename op_put_getter_setter to op_put_getter_setter_by_id
Comment 1 Yusuke Suzuki 2015-11-01 06:04:32 PST 
Created attachment 264514 [details]
Patch
Comment 2 Mark Lam 2015-11-01 06:38:22 PST 
Comment on attachment 264514 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        Renaming op_put_getter_setter to op_put_getter_setter_by_id makes this op name consistent to

"consistent to" ==> "consistent with"
Comment 3 Yusuke Suzuki 2015-11-01 08:12:10 PST 
Comment on attachment 264514 [details]
Patch

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

Thank you for your review :D

>> Source/JavaScriptCore/ChangeLog:8
>> +        Renaming op_put_getter_setter to op_put_getter_setter_by_id makes this op name consistent to
> 
> "consistent to" ==> "consistent with"

Thanks! Fixed.
Comment 4 Yusuke Suzuki 2015-11-01 08:18:49 PST 
Committed r191858: <http://trac.webkit.org/changeset/191858>
Comment 5 Alexey Proskuryakov 2015-11-01 14:34:14 PST 
This broke 32-bit build:

https://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/13856/steps/compile-webkit/logs/stdio

Undefined symbols for architecture i386:
  "_llint_slow_path_put_getter_setter", referenced from:
      _llint_entry in LowLevelInterpreter.o
     (maybe you meant: _llint_slow_path_put_getter_setter_by_id)
Comment 6 WebKit Commit Bot 2015-11-01 14:37:52 PST 
Re-opened since this is blocked by bug 150780
Comment 7 Alexey Proskuryakov 2015-11-01 14:44:41 PST 
Rolling out. Even if this just needs a can rebuild, the right fix is to fix the dependencies.
Comment 8 Yusuke Suzuki 2015-11-01 17:42:49 PST 
Hm,

```
offlineasm: Parsing /Volumes/Data/slave/mavericks-32bitJSC-debug/build/Source/JavaScriptCore/llint/LowLevelInterpreter.asm and creating offset extractor /Volumes/Data/slave/mavericks-32bitJSC-debug/build/WebKitBuild/Debug/LLIntOffsets/LLIntDesiredOffsets.h.
offlineasm: Nothing changed.
```

```
offlineasm: Parsing JavaScriptCore/llint/LowLevelInterpreter.asm and /Volumes/Data/slave/mavericks-32bitJSC-debug/build/WebKitBuild/Debug/JSCLLIntOffsetsExtractor and creating assembly file LLIntAssembly.h.
offlineasm: Nothing changed.
```

LowLevelInterpreter.asm is updated in the patch, but the output seems not updated.
Comment 9 Yusuke Suzuki 2015-11-01 21:02:57 PST 
Alexey, I think the patch itself is correct.
It changes all the existing getter_setter signature to getter_setter_by_id.

Seeing the log,
https://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/13853/steps/compile-webkit/logs/stdio
LowLevelInterpreter.o is not recompiled regardless of change of LLIntAssembly.h.
Do you know how XCode handles header change?
I'm not sure that Xcode correctly handles changes of headers.

One possible workaround is, adding white space line to LowLevelInterpreter.cpp.
Comment 10 Yusuke Suzuki 2015-11-01 21:26:53 PST 
(In reply to comment #9)
> One possible workaround is, adding white space line to
> LowLevelInterpreter.cpp.

But if do so, just issuing a clean build is preferable I think.
Comment 11 Yusuke Suzuki 2015-11-02 09:30:56 PST 
After investigating a little bit, I think adding LLIntAssembly.h into Xcode project file may solve this issue.
Comment 12 Yusuke Suzuki 2015-11-02 10:22:34 PST 
Discussed with Alexey on IRC.
We will include LLIntAssembly.h into Xcode project file, land the patch and watch on buildbot!
Comment 13 Yusuke Suzuki 2015-11-02 10:33:52 PST 
Committed r191897: <http://trac.webkit.org/changeset/191897>