Bug 150773

Summary: Rename op_put_getter_setter to op_put_getter_setter_by_id
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, keith_miller, mark.lam, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 150780    
Bug Blocks:    
Attachments:
Description Flags
Patch mark.lam: review+

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>