Bug 235275 - Do not use pas utils outside of libpas
Summary: Do not use pas utils outside of libpas
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: 230841
Blocks:
  Show dependency treegraph
 
Reported: 2022-01-15 15:39 PST by Yusuke Suzuki
Modified: 2022-01-19 13:49 PST (History)
6 users (show)

See Also:


Attachments
Patch (5.81 KB, patch)
2022-01-15 15:40 PST, 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 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