The Count/MaxCount used in TransferBatch and PerClass can be fit in u16 in
current configurations and it's also reasonable to have a u16 limit. The
spare 16 bits will be used for additional status like pages mapping
status in a TransferBatch.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
While this is true, making the Counts smaller can have structure alignement consequences.
You probably want to make sure that TransferBatch, FreeBatch, PreClass etc all look fine (size multiple of sizeof(uptr) etc). If not the case, you would have to pad.
Other things to keep in consideration: u32 vs u16 in getMaxCachedHint, TransferBatch::get and others (static_cast<u32> might be a giveaway of some integer size inconsitency).
If all works out, that might mean that the SizeClassConfig will have to be adapted to best fit the new sizes.
compiler-rt/lib/scudo/standalone/local_cache.h | ||
---|---|---|
38 | I should probably be a u16 as well for consitency, and this might mean propagating it to the callers as well | |
131 | PerClass might not be properly aligned anymore on 64-bit archs? | |
compiler-rt/lib/scudo/standalone/size_class_map.h | ||
38 | N is a u32 vs the result that is a u16. | |
compiler-rt/lib/scudo/standalone/tests/release_test.cpp | ||
139 | Probably u16? | |
140 | Same |
compiler-rt/lib/scudo/standalone/local_cache.h | ||
---|---|---|
131 | I added an alignas(SCUDO_CACHE_LINE_SIZE) here. I may want to reorder the fields to make the one with higher alignment requirements come first. Will do that in the later CL if that's a good idea. | |
compiler-rt/lib/scudo/standalone/size_class_map.h | ||
38 | The inner cast is replaced by explicit template argument |
Only some small comment nits.
compiler-rt/lib/scudo/standalone/primary32.h | ||
---|---|---|
385–386 | Small nit, should be result will also fit in u16. | |
compiler-rt/lib/scudo/standalone/primary64.h | ||
395–396 | Same as above "will also fit". | |
compiler-rt/lib/scudo/standalone/size_class_map.h | ||
35 | Might be better worded as "guaranteed to fit" | |
36 | Is this supposed to be aware? I don't quite understand the comment, maybe it needs to be reworded slightly. |
Address review comments
compiler-rt/lib/scudo/standalone/size_class_map.h | ||
---|---|---|
36 | Right, it should be "aware". Because the result of Max() is u32. Without the static_cast, it'll be an implicit truncation. I wanted to emphasize that "we know the truncation will happen" by using explicit casting. I think the previous sentence is enough. Removed the later one. |
LGTM
Any other reviewers, please take a quick look to make everything looks okay. If you feel that you might not understand all of the deeper changes, we'd still appreciate a higher level review to make sure there are no obvious errors.
@Chia-hungDuan, your change is causing a build failure on our internal bots that run older versions of gcc, specifically 7.5.0 and 9.4.0. It fails with the following error:
/home/dyung/src/upstream/llvm_clean_git/compiler-rt/lib/scudo/standalone/local_cache.h:190:14: error: conversion from ‘int’ to ‘scudo::u16’ {aka ‘short unsigned int’} may change value [-Werror=conversion] 190 | C->Count -= Count; | ~~~~~~~~~^~~~~~~~ /home/dyung/src/upstream/llvm_clean_git/compiler-rt/lib/scudo/standalone/local_cache.h:154:23: error: conversion from ‘int’ to ‘scudo::u16’ {aka ‘short unsigned int’} may change value [-Werror=conversion] 154 | P->MaxCount = 2 * TransferBatch::getMaxCached(Size); | ~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Can you take a look and revert if it will take you a while to fix?
This looks weird to me. Both of them are u16 already. Can you check why it thinks C-Count is int?
/home/dyung/src/upstream/llvm_clean_git/compiler-rt/lib/scudo/standalone/local_cache.h:154:23: error: conversion from ‘int’ to ‘scudo::u16’ {aka ‘short unsigned int’} may change value [-Werror=conversion]
154 | P->MaxCount = 2 * TransferBatch::getMaxCached(Size); | ~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
https://reviews.llvm.org/D135945. I think this will resolve the warning.
Can you take a look and revert if it will take you a while to fix?
There are also two additional failures for this reason that I think were introduced in follow-up commits:
/home/siadmin/jenkins/w/opensource/opensource_build/working_dir/compiler-rt/lib/scudo/standalone/primary64.h:445:49: error: conversion to 'scudo::u16 {aka short unsigned int}' from 'int' may alter its value [-Werror=conversion] u16 UnusedSlots = BG->MaxCachedPerBatch - CurBatch->getCount(); /home/siadmin/jenkins/w/opensource/opensource_build/working_dir/compiler-rt/lib/scudo/standalone/local_cache.h:34:13: error: conversion to 'scudo::u16 {aka short unsigned int}' from 'int' may alter its value [-Werror=conversion] Count += N; ~~~~~~^~~~
This is odd, I'm trying to figure out why this is a problem. If I had to guess, since scudo::u16 is a custom type, maybe C++ is generating a conversion implicitly for the arithmetic operators?
we are seeing the same issue compiling compiler-rt with these patches in trunk, are the various buildbots catching this also?
for awareness: @saiislam
Hi @Chia-hungDuan, The patch breaks the build on a supported compiler and two people have now noticed it, so we're thinking of reverting it because has been broken for 16 hours. Can you please confirm if you're actively looking at it?
I'm looking at this issue and the PPC bots issue now. I'll revert them if there's no easy fix for this.
https://reviews.llvm.org/D135945 is supposed to fix the warning. @ronlieb, @n-omer, could you help on verifying this? Thanks
I should probably be a u16 as well for consitency, and this might mean propagating it to the callers as well