Bug 186196 - [JSC] Correct values and members of JSBigInt appropriately
Summary: [JSC] Correct values and members of JSBigInt appropriately
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: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-01 08:54 PDT by Yusuke Suzuki
Modified: 2018-06-01 12:01 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.91 KB, patch)
2018-06-01 08:56 PDT, Yusuke Suzuki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2018-06-01 08:54:38 PDT
[JSC] Correct values and members of JSBigInt appropriately
Comment 1 Yusuke Suzuki 2018-06-01 08:56:19 PDT
Created attachment 341763 [details]
Patch
Comment 2 Darin Adler 2018-06-01 10:26:33 PDT
Comment on attachment 341763 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        2. JSBigInt::allocationSize should be annotated with `inline`.

Does this have any effect? Recently I’ve discovered that for the latest versions of clang, at least, "inline" really means "permission to define this in multiple translation units" or in other words "this function is in a header", and isn’t having an effect on what actually gets inlined.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:1309
> -    ASSERT(msdTopBit == (xBitLength - 1) % digitBits);
> +    ASSERT(msdTopBit == static_cast<int>((xBitLength - 1) % digitBits));

Why this change?
Comment 3 Yusuke Suzuki 2018-06-01 11:46:41 PDT
Comment on attachment 341763 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:11
>> +        2. JSBigInt::allocationSize should be annotated with `inline`.
> 
> Does this have any effect? Recently I’ve discovered that for the latest versions of clang, at least, "inline" really means "permission to define this in multiple translation units" or in other words "this function is in a header", and isn’t having an effect on what actually gets inlined.

Yeah, in GCC, this is effective. Maybe, this is because this `allocationSize()` function is static class member function, so it can be called outside of JSBigInt.cpp.
This `inline` prevents `allocationSize()` function from being called outside of this JSBigInt.cpp.

  c6400e:       48 8b 87 d0 ff 00 00    mov    0xffd0(%rdi),%rax
  c64015:       48 89 fd                mov    %rdi,%rbp
  c64018:       48 85 c0                test   %rax,%rax
  c6401b:       74 03                   je     c64020 <JSC::JSBigInt::createWithLength(JSC::VM&, unsigned int)+0x20>
  c6401d:       4c 8b 28                mov    (%rax),%r13
  c64020:       44 89 e7                mov    %r12d,%edi
  c64023:       e8 68 69 70 ff          callq  36a990 <JSC::JSBigInt::allocationSize(unsigned int)@plt>
  c64028:       48 3d 60 1f 00 00       cmp    $0x1f60,%rax
  c6402e:       76 50                   jbe    c64080 <JSC::JSBigInt::createWithLength(JSC::VM&, unsigned int)+0x80>
  c64030:       48 8d bd 40 36 00 00    lea    0x3640(%rbp),%rdi
  c64037:       45 31 c0                xor    %r8d,%r8d
  c6403a:       31 c9                   xor    %ecx,%ecx
  c6403c:       48 89 c2                mov    %rax,%rdx

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:1309
>> +    ASSERT(msdTopBit == static_cast<int>((xBitLength - 1) % digitBits));
> 
> Why this change?

It emits warnings with GCC since this comparison is signed v.s. unsigned.
Comment 4 Yusuke Suzuki 2018-06-01 11:47:32 PDT
Committed r232401: <https://trac.webkit.org/changeset/232401>
Comment 5 Radar WebKit Bug Importer 2018-06-01 12:01:18 PDT
<rdar://problem/40729485>