They became huge numbers which means the overflow happens at some point.
git bisect says the break happens on this commit: https://github.com/WebKit/WebKit/commit/8e1ec1def4ca1b38f2fad2679d120adc9531ce08#diff-34efcb839f5ce590e8868d478f3801379e2c680f863116a740b63b99598691d6
(In reply to Basuke Suzuki from comment #1) > git bisect says the break happens on this commit: > https://github.com/WebKit/WebKit/commit/ > 8e1ec1def4ca1b38f2fad2679d120adc9531ce08#diff- > 34efcb839f5ce590e8868d478f3801379e2c680f863116a740b63b99598691d6 But this change is correct because newly allocated range by VM has entire physical page. So the bug exists before this commit and this commit made that bug visible.
Created attachment 438160 [details] PATCH
Comment on attachment 438160 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=438160&action=review > Source/bmalloc/bmalloc/Heap.cpp:585 > + adjustFootprint(lock, range.totalPhysicalSize(), "allocateLarge"); These two lines are the fix.
Comment on attachment 438160 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=438160&action=review > Source/bmalloc/bmalloc/Heap.cpp:112 > + constexpr bool verbose = false; This constexpr controls the above logic and turning this false to delete the added complexity at all. Proven in compiler explorer: https://godbolt.org/z/eoK54xzq9
Comment on attachment 438160 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=438160&action=review > Source/bmalloc/bmalloc/Heap.cpp:100 > + BUNUSED(label); > + BUNUSED(note); label and note are used, so these statements appear to be unnecessary / incorrect. > Source/bmalloc/bmalloc/Heap.cpp:103 > + BASSERT(label || (amount >= 0 && result >= value) || (amount < 0 && result < value)); Why does having a label opt you out of this assertion? Side note: Are you trying to do checked arithmetic here? If so, I'd prefer using something from CheckedArithmetic.h. >> Source/bmalloc/bmalloc/Heap.cpp:112 >> + constexpr bool verbose = false; > > This constexpr controls the above logic and turning this false to delete the added complexity at all. Proven in compiler explorer: https://godbolt.org/z/eoK54xzq9 Does putting verbose inside Heap::adjustStat() not achieve the same goal, even though it is inlined? > Source/bmalloc/bmalloc/Heap.cpp:113 > + BUNUSED(note); Do we need this? 'note' appears in this function. > Source/bmalloc/bmalloc/Heap.cpp:126 > + BUNUSED(note); Ditto
Comment on attachment 438160 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=438160&action=review >> Source/bmalloc/bmalloc/Heap.cpp:100 >> + BUNUSED(note); > > label and note are used, so these statements appear to be unnecessary / incorrect. Right. This was leftover from pre-cleanup code. >> Source/bmalloc/bmalloc/Heap.cpp:103 >> + BASSERT(label || (amount >= 0 && result >= value) || (amount < 0 && result < value)); > > Why does having a label opt you out of this assertion? > > Side note: Are you trying to do checked arithmetic here? If so, I'd prefer using something from CheckedArithmetic.h. For debugging purpose. I wanted to see the weird overflow using verbose output. On that case this assertion was not required. But that can be controlled by release/debug. I can remove this. >>> Source/bmalloc/bmalloc/Heap.cpp:112 >>> + constexpr bool verbose = false; >> >> This constexpr controls the above logic and turning this false to delete the added complexity at all. Proven in compiler explorer: https://godbolt.org/z/eoK54xzq9 > > Does putting verbose inside Heap::adjustStat() not achieve the same goal, even though it is inlined? This can control activating only footprint verbose output, for instance. If the result after optimization differ for moving this into adjustStat() then I will consider for moving, but they are practically same. Or I can extract the logging part from adjustStat and call separately for clarification. >> Source/bmalloc/bmalloc/Heap.cpp:113 >> + BUNUSED(note); > > Do we need this? 'note' appears in this function. Leftover. will remove. >> Source/bmalloc/bmalloc/Heap.cpp:126 >> + BUNUSED(note); > > Ditto Ditto. Thanks
Created attachment 438316 [details] patch again. Porting CheckedArithmetic from WTF is too big to include this patch. That should be separated bug to be achieved.
<rdar://problem/83339024>
> Porting CheckedArithmetic from WTF is too big to include this patch. That > should be separated bug to be achieved. Fair point. I had forgotten that CheckedArithmetic was in a separate library.
Comment on attachment 438316 [details] patch again. View in context: https://bugs.webkit.org/attachment.cgi?id=438316&action=review r=me > Source/bmalloc/ChangeLog:3 > + [bmalloc] freeableMemory and footprint of Heap are broken Can you make this title more precise? Are freeableMemory and footprint literally completely broken in all cases all the time, or only in certain cases, or only broken in a particular way? Can you say anything about how observable behavior changes after this change is made?
Created attachment 438876 [details] PATCH Added example of output to display error information
> r=me Thank you very much. > > Source/bmalloc/ChangeLog:3 > > + [bmalloc] freeableMemory and footprint of Heap are broken > > Can you make this title more precise? Are freeableMemory and footprint > literally completely broken in all cases all the time, or only in certain > cases, or only broken in a particular way? > > Can you say anything about how observable behavior changes after this change > is made? I've added those description in ChangeLog,
Committed r282850 (241981@main): <https://commits.webkit.org/241981@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 438876 [details].
We started seeing many unreproducible crashes in bmalloc::Heap::decommitLargeRange on the bots recently, such as below. May need to revert this change to see if that fixes those. https://bug-230719-attachments.webkit.org/attachment.cgi?id=439093 https://bug-230866-attachments.webkit.org/attachment.cgi?id=439406
Reverted r282850 for reason: Patch causing many crashes in bmalloc::Heap::decommitLargeRange Committed r283264 (242293@main): <https://commits.webkit.org/242293@main>
(In reply to Alexey Proskuryakov from comment #15) > We started seeing many unreproducible crashes in > bmalloc::Heap::decommitLargeRange on the bots recently, such as below. May > need to revert this change to see if that fixes those. > > https://bug-230719-attachments.webkit.org/attachment.cgi?id=439093 > https://bug-230866-attachments.webkit.org/attachment.cgi?id=439406 Sure. Please do it. But I don't have idea why this causes the crash.
Wait a minute, are those debug build? If so, then that might be because I've added assertions there.
Yes, these look like assertions (at least two separate ones, judging from crash logs).
(In reply to Alexey Proskuryakov from comment #19) > Yes, these look like assertions (at least two separate ones, judging from > crash logs). Give me an advice. Here's the background. 1. The numbers are broken before this patch landed 2. Those numbers are debugging information which is not used at runtime (as far as I know) 3. I fixed the one I've found and it seems the number is now healthy. 4. I've added assertion to detect future regression 5. Actually there's still case where the numbers are incorrect. 6. I basically need this fixed number to see my other patches effect. I think to remove assertion is the best for now. What do you think?
Created attachment 439770 [details] PATCH Make assertion optional.
Comment on attachment 439770 [details] PATCH The optional assertion in adjustStat is required to find the hidden issue.
Comment on attachment 439770 [details] PATCH r=me What's our plan to track down the remaining bugs and re-enable the assertion?
Committed r286029 (244417@main): <https://commits.webkit.org/244417@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 439770 [details].
Comment on attachment 439770 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=439770&action=review > Source/bmalloc/ChangeLog:9 > + This introduced in r279922. The physical size of the newly allocated range was changed from zero Can we make API tests that would detect if we break this again in future?