Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1112003002:
Use 64 bit shift when convert uint64_t to ByteVector (Closed)
DescriptionUse 64 bit shift when convert uint64_t to ByteVector
Seperates this step out and adds a test for it.
BUG=427616
Committed: https://crrev.com/d1fbbe8536b7965c6ea7e61296b0cbace12ec702
Cr-Commit-Position: refs/heads/master@{#327787}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Use size_t #Patch Set 3 : Remove cast for negative testing #Patch Set 4 : Restore the cast #
Messages
Total messages: 20 (3 generated)
holte@chromium.org changed reviewers: + asvitkine@chromium.org, holte@chromium.org
This works fine for me locally even without the cast, but it's generating warnings on other builds.
On 2015/04/29 01:12:24, Steven Holte wrote: > This works fine for me locally even without the cast, but it's generating > warnings on other builds. See brucedawson@ comment on https://codereview.chromium.org/1090683003/ for context
Have you confirmed that the test fails on Windows without the fix? https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... File components/rappor/byte_vector_utils.cc (right): https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... components/rappor/byte_vector_utils.cc:87: uint64_t byte_mask = uint64_t(0xff) << shift; Our style guide prefers static_cast<uint64_t>(). https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... File components/rappor/byte_vector_utils.h (right): https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... components/rappor/byte_vector_utils.h:21: void Uint64ToByteVector(uint64_t value, uint64_t size, ByteVector* output); Why is size a uint64_t? Should just be an int.
I haven't tested pre-fix on windows, since I don't have windows machine handy. https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... File components/rappor/byte_vector_utils.cc (right): https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... components/rappor/byte_vector_utils.cc:87: uint64_t byte_mask = uint64_t(0xff) << shift; On 2015/04/29 15:01:07, Alexei Svitkine wrote: > Our style guide prefers static_cast<uint64_t>(). Done. https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... File components/rappor/byte_vector_utils.h (right): https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... components/rappor/byte_vector_utils.h:21: void Uint64ToByteVector(uint64_t value, uint64_t size, ByteVector* output); On 2015/04/29 15:01:07, Alexei Svitkine wrote: > Why is size a uint64_t? Should just be an int. Changed to size_t
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
> This works fine for me locally even without the cast, but > it's generating warnings on other builds. This confuses me. If you take 0xFF and shift it by more than 32 then you get undefined behavior, and a result that cannot be greater than 32 bits. The /analyze warning is Windows specific, but the underlying bug should be universal. If the newly added test passes without the cast then I'm surprised and confused. I just tested with g++ and confirmed that shifting 0xFF left by 40 causes overflow, not a 64-bit result. https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... File components/rappor/byte_vector_utils.cc (right): https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... components/rappor/byte_vector_utils.cc:86: uint64_t shift = i * 8; Why is shift a uint64_t? The legal values are from 0-63 so an int or unsigned is more than sufficient.
You can always use try bots to test on platforms other than the ones that match your local one. On Wed, Apr 29, 2015 at 2:21 PM, <brucedawson@chromium.org> wrote: > This works fine for me locally even without the cast, but >> it's generating warnings on other builds. >> > > This confuses me. If you take 0xFF and shift it by more than 32 then you > get > undefined behavior, and a result that cannot be greater than 32 bits. The > /analyze warning is Windows specific, but the underlying bug should be > universal. If the newly added test passes without the cast then I'm > surprised > and confused. > > I just tested with g++ and confirmed that shifting 0xFF left by 40 causes > overflow, not a 64-bit result. > > > > https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... > File components/rappor/byte_vector_utils.cc (right): > > > https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... > components/rappor/byte_vector_utils.cc:86: uint64_t shift = i * 8; > Why is shift a uint64_t? The legal values are from 0-63 so an int or > unsigned is more than sufficient. > > https://codereview.chromium.org/1112003002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The /analyze warnings (which are what found the bug) are on a .fyi bot rather than a try bot, and they have a 8-12 hour turnaround, so don't wait on those. Luckily the behavior is portable, only the warning is not, so if the code works on Linux it should work with VC++. On Wed, Apr 29, 2015 at 11:23 AM, Alexei Svitkine <asvitkine@chromium.org> wrote: > You can always use try bots to test on platforms other than the ones that > match your local one. > > On Wed, Apr 29, 2015 at 2:21 PM, <brucedawson@chromium.org> wrote: > >> This works fine for me locally even without the cast, but >>> it's generating warnings on other builds. >>> >> >> This confuses me. If you take 0xFF and shift it by more than 32 then you >> get >> undefined behavior, and a result that cannot be greater than 32 bits. The >> /analyze warning is Windows specific, but the underlying bug should be >> universal. If the newly added test passes without the cast then I'm >> surprised >> and confused. >> >> I just tested with g++ and confirmed that shifting 0xFF left by 40 causes >> overflow, not a 64-bit result. >> >> >> >> https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... >> File components/rappor/byte_vector_utils.cc (right): >> >> >> https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... >> components/rappor/byte_vector_utils.cc:86: uint64_t shift = i * 8; >> Why is shift a uint64_t? The legal values are from 0-63 so an int or >> unsigned is more than sufficient. >> >> https://codereview.chromium.org/1112003002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I meant trying the unit test without the fix on the bots - to check if it fails, not the /analyze check. On Wed, Apr 29, 2015 at 2:27 PM, Bruce Dawson <brucedawson@chromium.org> wrote: > The /analyze warnings (which are what found the bug) are on a .fyi bot > rather than a try bot, and they have a 8-12 hour turnaround, so don't wait > on those. > > Luckily the behavior is portable, only the warning is not, so if the code > works on Linux it should work with VC++. > > On Wed, Apr 29, 2015 at 11:23 AM, Alexei Svitkine <asvitkine@chromium.org> > wrote: > >> You can always use try bots to test on platforms other than the ones that >> match your local one. >> >> On Wed, Apr 29, 2015 at 2:21 PM, <brucedawson@chromium.org> wrote: >> >>> This works fine for me locally even without the cast, but >>>> it's generating warnings on other builds. >>>> >>> >>> This confuses me. If you take 0xFF and shift it by more than 32 then you >>> get >>> undefined behavior, and a result that cannot be greater than 32 bits. The >>> /analyze warning is Windows specific, but the underlying bug should be >>> universal. If the newly added test passes without the cast then I'm >>> surprised >>> and confused. >>> >>> I just tested with g++ and confirmed that shifting 0xFF left by 40 causes >>> overflow, not a 64-bit result. >>> >>> >>> >>> https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... >>> File components/rappor/byte_vector_utils.cc (right): >>> >>> >>> https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... >>> components/rappor/byte_vector_utils.cc:86: uint64_t shift = i * 8; >>> Why is shift a uint64_t? The legal values are from 0-63 so an int or >>> unsigned is more than sufficient. >>> >>> https://codereview.chromium.org/1112003002/ >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Gotcha. Yes, verifying that the test fails on all platforms would be good. It should. On Wed, Apr 29, 2015 at 11:29 AM, Alexei Svitkine <asvitkine@chromium.org> wrote: > I meant trying the unit test without the fix on the bots - to check if it > fails, not the /analyze check. > > On Wed, Apr 29, 2015 at 2:27 PM, Bruce Dawson <brucedawson@chromium.org> > wrote: > >> The /analyze warnings (which are what found the bug) are on a .fyi bot >> rather than a try bot, and they have a 8-12 hour turnaround, so don't wait >> on those. >> >> Luckily the behavior is portable, only the warning is not, so if the code >> works on Linux it should work with VC++. >> >> On Wed, Apr 29, 2015 at 11:23 AM, Alexei Svitkine <asvitkine@chromium.org >> > wrote: >> >>> You can always use try bots to test on platforms other than the ones >>> that match your local one. >>> >>> On Wed, Apr 29, 2015 at 2:21 PM, <brucedawson@chromium.org> wrote: >>> >>>> This works fine for me locally even without the cast, but >>>>> it's generating warnings on other builds. >>>>> >>>> >>>> This confuses me. If you take 0xFF and shift it by more than 32 then >>>> you get >>>> undefined behavior, and a result that cannot be greater than 32 bits. >>>> The >>>> /analyze warning is Windows specific, but the underlying bug should be >>>> universal. If the newly added test passes without the cast then I'm >>>> surprised >>>> and confused. >>>> >>>> I just tested with g++ and confirmed that shifting 0xFF left by 40 >>>> causes >>>> overflow, not a 64-bit result. >>>> >>>> >>>> >>>> https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... >>>> File components/rappor/byte_vector_utils.cc (right): >>>> >>>> >>>> https://codereview.chromium.org/1112003002/diff/1/components/rappor/byte_vect... >>>> components/rappor/byte_vector_utils.cc:86: uint64_t shift = i * 8; >>>> Why is shift a uint64_t? The legal values are from 0-63 so an int or >>>> unsigned is more than sufficient. >>>> >>>> https://codereview.chromium.org/1112003002/ >>>> >>> >>> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I added BUG=427616 to the description -- that's the /analyze blanket bug.
It definitely does not fail on all platforms, since as I stated earlier it passes without the cast on my local linux build. On Wed, Apr 29, 2015 at 11:42 AM, <brucedawson@chromium.org> wrote: > I added BUG=427616 to the description -- that's the /analyze blanket bug. > > https://codereview.chromium.org/1112003002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I saw that and I don't understand how that can be true. I hacked up some
test code to verify that shifting 0xFF left by 40 does not generate a
64-bit result. If the tests are passing that suggests that they don't
depend on that, in which case maybe the result should be a 32-bit type?
Here's my test code, build command, and the results, FWIW:
main.cpp:
#include <stdio.h>
typedef unsigned long long uint64_t;
uint64_t shift = 40;
int main(int argc, char* argv[])
{
uint64_t result = 0xff << shift;
printf("%llu is the answer.\n", result);
return 0;
}
$ g++ main.cpp
$ file a.out
a.out: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically
linked (uses shared libs), for GNU/Linux 2.6.24,
BuildID[sha1]=353784c25e9aeca3d6a4b866e5dbf8cf9db95fa4, not stripped
$ ./a.out
65280 is the answer.
On Wed, Apr 29, 2015 at 12:14 PM, Steven Holte <holte@google.com> wrote:
> It definitely does not fail on all platforms, since as I stated earlier it
> passes without the cast on my local linux build.
>
> On Wed, Apr 29, 2015 at 11:42 AM, <brucedawson@chromium.org> wrote:
>
>> I added BUG=427616 to the description -- that's the /analyze blanket bug.
>>
>> https://codereview.chromium.org/1112003002/
>>
>
>
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
On 2015/04/29 20:40:03, brucedawson wrote:
> I saw that and I don't understand how that can be true. I hacked up some
> test code to verify that shifting 0xFF left by 40 does not generate a
> 64-bit result. If the tests are passing that suggests that they don't
> depend on that, in which case maybe the result should be a 32-bit type?
>
> Here's my test code, build command, and the results, FWIW:
>
> main.cpp:
> #include <stdio.h>
>
> typedef unsigned long long uint64_t;
>
> uint64_t shift = 40;
>
> int main(int argc, char* argv[])
> {
> uint64_t result = 0xff << shift;
> printf("%llu is the answer.\n", result);
> return 0;
> }
>
> $ g++ main.cpp
> $ file a.out
> a.out: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically
> linked (uses shared libs), for GNU/Linux 2.6.24,
> BuildID[sha1]=353784c25e9aeca3d6a4b866e5dbf8cf9db95fa4, not stripped
> $ ./a.out
> 65280 is the answer.
>
>
> On Wed, Apr 29, 2015 at 12:14 PM, Steven Holte <mailto:holte@google.com>
wrote:
>
> > It definitely does not fail on all platforms, since as I stated earlier it
> > passes without the cast on my local linux build.
> >
> > On Wed, Apr 29, 2015 at 11:42 AM, <mailto:brucedawson@chromium.org> wrote:
> >
> >> I added BUG=427616 to the description -- that's the /analyze blanket bug.
> >>
> >> https://codereview.chromium.org/1112003002/
> >>
> >
> >
>
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
It looks like I'm bad at guessing which try jobs actually run
component_unittests, but I did manage to get both a passing and failing build
for my test. It looks like in the linux_chromium_chromeos_rel_ng build it
passes, and in linux_android_dbg_ng it fails.
lgtm
The CQ bit was checked by holte@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1112003002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d1fbbe8536b7965c6ea7e61296b0cbace12ec702 Cr-Commit-Position: refs/heads/master@{#327787} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
