Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2775663003:
[memory-infra] Fix base_unittests on CFI bots (Closed)
Description[memory-infra] Fix base_unittests on CFI bots
Sometime integer overflow happens on LTO builds. Add a cast to int64
BUG=654667
Review-Url: https://codereview.chromium.org/2775663003
Cr-Commit-Position: refs/heads/master@{#459391}
Committed: https://chromium.googlesource.com/chromium/src/+/8cf5cdc8b86d8370d9b56eb54b91b2e6d1d51cc5
Patch Set 1 #
Depends on Patchset: Messages
Total messages: 15 (8 generated)
ssid@chromium.org changed reviewers: + krasin@chromium.org, primiano@chromium.org
primiano, ptal, small change, thanks. The build error shows: ../../base/trace_event/memory_dump_scheduler_unittest.cc:95: Failure Expected: (static_cast<int64_t>(total - mds_->polling_state_.last_dump_memory_total)) < (mds_->polling_state_.memory_increase_threshold), actual: 0 vs -2097340416 I am assuming that the threshold is negative. So, added cast and dcheck.
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM but I am not sure that is really the problem. Even if the machine has 128 GB of memory, mem.total will be: int total = 128 * 1024 * 1024; //mem.total is in kB then you do total / 100 * 1024, which would be still < the 2BG wrapping limit. So either the machine that failed has >= 256 GB of memory or the problem is somewhere else? In any case, this looks good regardelss.
The CQ bit was checked by primiano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1490355324389450, "parent_rev":
"419fb6f42e0ead5bfc5983fddb6f404f835f05be", "commit_rev":
"8cf5cdc8b86d8370d9b56eb54b91b2e6d1d51cc5"}
Message was sent while issue was closed.
Description was changed from ========== [memory-infra] Fix base_unittests on CFI bots Sometime integer overflow happens on LTO builds. Add a cast to int64 BUG=654667 ========== to ========== [memory-infra] Fix base_unittests on CFI bots Sometime integer overflow happens on LTO builds. Add a cast to int64 BUG=654667 Review-Url: https://codereview.chromium.org/2775663003 Cr-Commit-Position: refs/heads/master@{#459391} Committed: https://chromium.googlesource.com/chromium/src/+/8cf5cdc8b86d8370d9b56eb54b91... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/8cf5cdc8b86d8370d9b56eb54b91...
Message was sent while issue was closed.
On 2017/03/24 11:35:17, Primiano Tucci wrote: > LGTM but I am not sure that is really the problem. > Even if the machine has 128 GB of memory, mem.total will be: > > int total = 128 * 1024 * 1024; //mem.total is in kB > then you do > total / 100 * 1024, which would be still < the 2BG wrapping limit. > So either the machine that failed has >= 256 GB of memory or the problem is > somewhere else? > > In any case, this looks good regardelss. The machine has 208 GB of RAM. So, it's like: int total = 256 * 1024 * 1024; // mem.total is in kB total / 100 * 1024 = 2748778496 > 2147483647 So, yes, the bug was real; it was only reproducible on LTO and CFI buildbots because only they have so much RAM, and the fix is correct (assuming all cases like these are updated) Trybots were unable to see the issue, because (for some unclear for me reason) they only have 118 GB RAM: https://build.chromium.org/p/tryserver.chromium.linux/buildslaves/slave904-c4 The actions to be considered: 1. Make LTO / CFI trybots and buildbots be more similar, in particular, they should have the same amount of RAM. See https://crbug.com/704962 2. UBSan buildbot should probably have signed integer overflow check enabled (that's already true: https://cs.chromium.org/chromium/src/build/config/sanitizers/BUILD.gn?type=cs...) and probably have 208 GB RAM. That's unlikely to happen: these machines are expensive, and next time the bug might actually be not related to the RAM size, so we might need to have both sizes; I don't know.
Message was sent while issue was closed.
Yeah from calculation it turned out the RAM size was 205GB. Maybe possible.
Message was sent while issue was closed.
On 2017/03/24 16:53:14, ssid wrote: > Yeah from calculation it turned out the RAM size was 205GB. Maybe possible. By the way, thank you for fixing this hard-to-spot bug, Siddhartha! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
