Do not use pas utils outside of libpas

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Do not use pas utils outside of libpas

[Yusuke Suzuki]

Created attachment 449276 [details]
Patch

[Darin Adler]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r288239 (246193@trunk): <https://commits.webkit.org/246193@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/87788695>

[Yusuke Suzuki]

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