Fix -Wshorten-64-to-32 warnings in zucchini |
||
Issue description
Compile zucchini with -Wshorten-64-to-32 enabled for x86_64 target:
[2788/37213] CXX obj/components/zucchini/zucchini_lib/equivalence_map.obj
../../components/zucchini/equivalence_map.cc(289,27): warning: implicit conversion loses integer precision: 'unsigned long long' to 'zucchini::offset_t' (aka 'unsigned int') [-Wshorten-64-to-32]
offset_t delta = offset - old_image_size_;
~~~~~ ~~~~~~~^~~~~~~~~~~~~~~~~
../../components/zucchini/equivalence_map.cc(290,67): warning: implicit conversion loses integer precision: 'unsigned long long' to 'zucchini::offset_t' (aka 'unsigned int') [-Wshorten-64-to-32]
return delta < kOffsetBound - new_image_size_ ? new_image_size_ + delta
~~~~~~ ~~~~~~~~~~~~~~~~^~~~~~~
2 warnings generated.
https://cs.chromium.org/chromium/src/components/zucchini/equivalence_map.cc?l=289
I tried fixing this naively, but I think this is more complex than I originally thought.
The first one seems trivial since |old_image_size_| must always fit in an offset_t because it's already been asserted to be smaller than |offset| which is an offset_t.
The second one I don't know what should happen here.
,
Sep 5
I think the fix should be making |old_image_size_| and |new_image_size_| both offset_t in the .h file, with checked_cast<> applied when we first assign the variables.
,
Sep 7
I'm applying this fix. I'm getting a few other errors in zucchini with -Wshorten-64-to-32 I'll need to fix as well.
,
Sep 7
okay thanks, bear in mind you might get warnings when compiling 32-bit - I haven't even started tackling those yet. for the first part of this enabling I'm only enabling on 64-bit builds (just to make it manageable).
,
Sep 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5946dbfa3f684d8f4960bb413b5e8322ebddcee3 commit 5946dbfa3f684d8f4960bb413b5e8322ebddcee3 Author: Etienne Pierre-doray <etiennep@chromium.org> Date: Mon Sep 10 16:19:33 2018 [Zucchini]: Fix OffsetMapper implicit conversion. Fix compile error with -Wshorten-64-to-32. Image size is new stored as an offset_t to avoid implicit conversion. Bug: 881008 Change-Id: I82b12ce17d8368f05d6a5537fd1734ee32b37dbe Reviewed-on: https://chromium-review.googlesource.com/1213549 Reviewed-by: Samuel Huang <huangs@chromium.org> Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org> Cr-Commit-Position: refs/heads/master@{#589938} [modify] https://crrev.com/5946dbfa3f684d8f4960bb413b5e8322ebddcee3/components/zucchini/equivalence_map.cc [modify] https://crrev.com/5946dbfa3f684d8f4960bb413b5e8322ebddcee3/components/zucchini/equivalence_map.h [modify] https://crrev.com/5946dbfa3f684d8f4960bb413b5e8322ebddcee3/components/zucchini/zucchini_apply.cc [modify] https://crrev.com/5946dbfa3f684d8f4960bb413b5e8322ebddcee3/components/zucchini/zucchini_gen.cc [modify] https://crrev.com/5946dbfa3f684d8f4960bb413b5e8322ebddcee3/components/zucchini/zucchini_gen_unittest.cc
,
Sep 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f6c70a2cc49b3a1765aa38763532e948c48a65a1 commit f6c70a2cc49b3a1765aa38763532e948c48a65a1 Author: Etienne Pierre-doray <etiennep@chromium.org> Date: Tue Sep 11 13:40:46 2018 [Zucchini]: Fix implicit conversions. Fix compile error with -Wshorten-64-to-32. Bug: 881008 Change-Id: I52a1bab9cb7b4cb67ea4c275143390d42b192120 Reviewed-on: https://chromium-review.googlesource.com/1216524 Reviewed-by: Will Harris <wfh@chromium.org> Reviewed-by: Samuel Huang <huangs@chromium.org> Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org> Cr-Commit-Position: refs/heads/master@{#590285} [modify] https://crrev.com/f6c70a2cc49b3a1765aa38763532e948c48a65a1/components/zucchini/disassembler_elf.cc [modify] https://crrev.com/f6c70a2cc49b3a1765aa38763532e948c48a65a1/components/zucchini/disassembler_ztf.cc [modify] https://crrev.com/f6c70a2cc49b3a1765aa38763532e948c48a65a1/components/zucchini/rel32_finder.cc [modify] https://crrev.com/f6c70a2cc49b3a1765aa38763532e948c48a65a1/components/zucchini/reloc_win32.cc
,
Sep 11
I believe the issue is fixed, at least for zucchini_lib. Is the goal to have -Wshorten-64-to-32 everywhere?
,
Sep 11
yes, that's being tracked in issue 879657 - the initial step will be enabling it for 64-bit builds. 32-bit builds will come a Little Later. Thanks for landing these fixes. |
||
►
Sign in to add a comment |
||
Comment 1 by wfh@chromium.org
, Sep 5