Bug 235275

Summary: Do not use pas utils outside of libpas
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, darin, ews-watchlist, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 230841    
Bug Blocks:    
Attachments:
Description Flags
Patch darin: review+

Description Yusuke Suzuki 2022-01-15 15:39:18 PST
Do not use pas utils outside of libpas
Comment 1 Yusuke Suzuki 2022-01-15 15:40:46 PST
Created attachment 449276 [details]
Patch
Comment 2 Darin Adler 2022-01-16 07:48:52 PST
Comment on attachment 449276 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        We should not use any utility functions from libpas outside of bmalloc.

Can we change how the headers are structured so the compile will fail too? That way the rule would “guard itself” and we would not need vigilant programmers thinking about this rule. Typically in separate frameworks the technique would be “mark header internal”; headers would not be installed. Is there a similar technique at least for the bmalloc/WebCore boundary if not bmalloc/WTF?

> Source/WTF/wtf/MathExtras.h:773
> +inline uint64_t reverseBits64(uint64_t value)

Why add this unused and untested function?

> Source/WTF/wtf/MathExtras.h:782
> +    return ((uint64_t)reverseBits32((unsigned)value) << (uint64_t)32)

I suggest uint32_t here instead of unsigned

> Source/WTF/wtf/MathExtras.h:783
> +        | (uint64_t)reverseBits32((unsigned)(value >> (uint64_t)32));

Ditto
Comment 3 Yusuke Suzuki 2022-01-19 13:28:58 PST
Comment on attachment 449276 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        We should not use any utility functions from libpas outside of bmalloc.
> 
> Can we change how the headers are structured so the compile will fail too? That way the rule would “guard itself” and we would not need vigilant programmers thinking about this rule. Typically in separate frameworks the technique would be “mark header internal”; headers would not be installed. Is there a similar technique at least for the bmalloc/WebCore boundary if not bmalloc/WTF?

Unfortunately, pas_utils.h is basis of libpas, and there are some legit use of pas_utils.h outside of libpas (JSC's jit_heap for example).
So we cannot prevent the outer projects from including it for now.

>> Source/WTF/wtf/MathExtras.h:773
>> +inline uint64_t reverseBits64(uint64_t value)
> 
> Why add this unused and untested function?

I copied these set of functions from libpas to prevent including libpas directly further, but for now, it is not used.
For the patch's simplicity, I'll just remove this.
Comment 4 Yusuke Suzuki 2022-01-19 13:35:13 PST
Committed r288239 (246193@trunk): <https://commits.webkit.org/246193@trunk>
Comment 5 Radar WebKit Bug Importer 2022-01-19 13:36:17 PST
<rdar://problem/87788695>
Comment 6 Yusuke Suzuki 2022-01-19 13:49:31 PST
Comment on attachment 449276 [details]
Patch

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

>>> Source/WebCore/ChangeLog:8
>>> +        We should not use any utility functions from libpas outside of bmalloc.
>> 
>> Can we change how the headers are structured so the compile will fail too? That way the rule would “guard itself” and we would not need vigilant programmers thinking about this rule. Typically in separate frameworks the technique would be “mark header internal”; headers would not be installed. Is there a similar technique at least for the bmalloc/WebCore boundary if not bmalloc/WTF?
> 
> Unfortunately, pas_utils.h is basis of libpas, and there are some legit use of pas_utils.h outside of libpas (JSC's jit_heap for example).
> So we cannot prevent the outer projects from including it for now.

It seems that, if we do that, it will break 32bit build. https://bugs.webkit.org/show_bug.cgi?id=235372