Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
613893002:
Fix more MSVC warnings, courgette/ edition. (Closed)
|
Created:
6 years, 2 months ago by Peter Kasting Modified:
6 years, 2 months ago Reviewers:
dgarrett CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionFix more MSVC warnings, courgette/ edition.
This is mostly about changing types and inserting casts so as to avoid implicit
value truncations.
BUG=81439
TEST=none
Committed: https://crrev.com/8e3a26aaf2392d784ba6d5cbb696da926230a722
Cr-Commit-Position: refs/heads/master@{#298069}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Review comments #Patch Set 3 : Remove dead code #Patch Set 4 : static_cast #
Messages
Total messages: 29 (6 generated)
pkasting@chromium.org changed reviewers: + dgarrett@chromium.org
See explanatory comments below. https://codereview.chromium.org/613893002/diff/1/courgette/adjustment_method_... File courgette/adjustment_method_2.cc (right): https://codereview.chromium.org/613893002/diff/1/courgette/adjustment_method_... courgette/adjustment_method_2.cc:652: uint8 kind = 0; Changed this because it's going to be stored back into |kinds_| (uint8[]) at the bottom of the loop. Changed the other two types to match. This is guaranteed to be safe; nothing in this loop can set these variables to anything beyond uint8_max. https://codereview.chromium.org/613893002/diff/1/courgette/assembly_program.cc File courgette/assembly_program.cc (right): https://codereview.chromium.org/613893002/diff/1/courgette/assembly_program.c... courgette/assembly_program.cc:93: BytesInstruction(const uint8* values, size_t len) These changes follow from the API change to EmitBytesInstruction(). https://codereview.chromium.org/613893002/diff/1/courgette/assembly_program.h File courgette/assembly_program.h (right): https://codereview.chromium.org/613893002/diff/1/courgette/assembly_program.h... courgette/assembly_program.h:90: CheckBool EmitBytesInstruction(const uint8* value, size_t len) Only called by DisassemblerElf32::ParseSimpleRegion(). On trunk that passes a ptrdiff_t here, but a size_t is more correct (and this CL changes it to size_t). It's not instantly clear to me whether downcasting from a size_t to uint32 at that callsite is safe, but why worry? Just consistently use size_t and we know there will be no problems. https://codereview.chromium.org/613893002/diff/1/courgette/disassembler_elf_3... File courgette/disassembler_elf_32.cc (left): https://codereview.chromium.org/613893002/diff/1/courgette/disassembler_elf_3... courgette/disassembler_elf_32.cc:410: const uint8* end = OffsetToPointer(end_file_offset); There's no real gain to converting the offsets to pointers, since OffsetToPointer() is always going to add the same value, so the math below works fine without doing this. https://codereview.chromium.org/613893002/diff/1/courgette/disassembler_elf_3... File courgette/disassembler_elf_32.cc (right): https://codereview.chromium.org/613893002/diff/1/courgette/disassembler_elf_3... courgette/disassembler_elf_32.cc:411: const size_t len = end_file_offset - start_file_offset; Since we know end > start, storing this as a size_t is safe, and conceptually, it's more correct to use a size_t than a ptrdiff_t for a length of some memory object. https://codereview.chromium.org/613893002/diff/1/courgette/disassembler_win32... File courgette/disassembler_win32_x64.cc (right): https://codereview.chromium.org/613893002/diff/1/courgette/disassembler_win32... courgette/disassembler_win32_x64.cc:576: // !!! Is this cast safe? This question must be answered before this change can be committed. It worries me that we're just blindly downcasting a uint64 here, especially when the comments on the declaration of image_base() suggest that it really can require > 32 bits. The assumption that this fits in 32 bits seems pretty baked in to the code here, so it's not clear to me what to do. https://codereview.chromium.org/613893002/diff/1/courgette/encoded_program.cc File courgette/encoded_program.cc (left): https://codereview.chromium.org/613893002/diff/1/courgette/encoded_program.cc... courgette/encoded_program.cc:50: T_must_fit_in_uint32); This assertion isn't correct. It's legal to pass in types larger than uint32 as long as they hold values that can be expressed in 32 bits -- something which WriteSizeVarint32() LOG_ASSERT()s already. https://codereview.chromium.org/613893002/diff/1/courgette/encoded_program.cc File courgette/encoded_program.cc (right): https://codereview.chromium.org/613893002/diff/1/courgette/encoded_program.cc... courgette/encoded_program.cc:569: current_rva += static_cast<RVA>(count); It's somewhat inconsistent for me to complain earlier about it not being obvious that casting a size_t to a uint32 at the callsite of EmitBytesInstruction() is safe, but then allow that same cast to happen here. I imagine changing RVA from a uint32 to a size_t is out of the question, so another possibility besides doing this is to use RVA (instead of size_t) for the type in the places I modified in this CL. Or perhaps the code here should do some sort of checked_cast or something. https://codereview.chromium.org/613893002/diff/1/courgette/encoded_program.h File courgette/encoded_program.h (right): https://codereview.chromium.org/613893002/diff/1/courgette/encoded_program.h#... courgette/encoded_program.h:44: CheckBool AddCopy(size_t count, const void* bytes) WARN_UNUSED_RESULT; More fallout from the EmitBytesInstruction() change, as are the other changes in this file.
Minus a couple of comments, I think this is a really good change. https://codereview.chromium.org/613893002/diff/1/courgette/adjustment_method_... File courgette/adjustment_method_2.cc (right): https://codereview.chromium.org/613893002/diff/1/courgette/adjustment_method_... courgette/adjustment_method_2.cc:652: uint8 kind = 0; Seems like Shingle::kWidth should also be updated to match. Is that awkward somehow? https://codereview.chromium.org/613893002/diff/1/courgette/disassembler_win32... File courgette/disassembler_win32_x64.cc (right): https://codereview.chromium.org/613893002/diff/1/courgette/disassembler_win32... courgette/disassembler_win32_x64.cc:576: // !!! Is this cast safe? There were repeated assurances that it is safe for Win64, but I can see how it might break. I think that adding a comment describing the concerns is very reasonable. An error check to blow up if the assumption is wrong it also reasonable. However, allowing the RVA sizes to vary based on platform would be a much larger change than than was done, and increasing them to 64 bits on all platforms would bloat patch sizes. I'd have to re-read code for a while to decide if getting this wrong will lead to invalid patches or just bloated patches. https://codereview.chromium.org/613893002/diff/1/courgette/encoded_program.cc File courgette/encoded_program.cc (right): https://codereview.chromium.org/613893002/diff/1/courgette/encoded_program.cc... courgette/encoded_program.cc:569: current_rva += static_cast<RVA>(count); I suspect that count should remain a uint32 here. That IS what's being read out of the encoded format.
https://codereview.chromium.org/613893002/diff/1/courgette/adjustment_method_... File courgette/adjustment_method_2.cc (right): https://codereview.chromium.org/613893002/diff/1/courgette/adjustment_method_... courgette/adjustment_method_2.cc:652: uint8 kind = 0; On 2014/10/01 20:49:37, dgarrett wrote: > Seems like Shingle::kWidth should also be updated to match. Is that awkward > somehow? It just involves touching more code, but you're right, it probably should be updated to prevent someone from making it >= UINT8_MAX in the future. Changed. https://codereview.chromium.org/613893002/diff/1/courgette/disassembler_win32... File courgette/disassembler_win32_x64.cc (right): https://codereview.chromium.org/613893002/diff/1/courgette/disassembler_win32... courgette/disassembler_win32_x64.cc:576: // !!! Is this cast safe? On 2014/10/01 20:49:37, dgarrett wrote: > I think that adding a comment describing the concerns is very > reasonable. An error check to blow up if the assumption is wrong it also > reasonable. OK. I implemented this by way of a checked_cast<>, which will CHECK if the assumption is violated. Let me know if you meant for failure in this case to do something less severe, like just returning false. https://codereview.chromium.org/613893002/diff/1/courgette/encoded_program.cc File courgette/encoded_program.cc (right): https://codereview.chromium.org/613893002/diff/1/courgette/encoded_program.cc... courgette/encoded_program.cc:569: current_rva += static_cast<RVA>(count); On 2014/10/01 20:49:37, dgarrett wrote: > I suspect that count should remain a uint32 here. That IS what's being read out > of the encoded format. ? |copy_counts_| is now a SizeTVector, so |count| really is a size_t. This goes hand-in-hand with changing AddCopy() to take a size_t, etc. As I said previously, it would also be possible to undo all the size_t changes (they're all connected) and use RVA everywhere, in which case the cast here would basically move to DisassemblerElf32::ParseSimpleRegion(). We could then decide to upgrade that cast to something like a checked_cast. (Or we could just upgrade the cast here.)
On 2014/10/01 22:12:05, Peter Kasting wrote: > https://codereview.chromium.org/613893002/diff/1/courgette/adjustment_method_... > File courgette/adjustment_method_2.cc (right): > > https://codereview.chromium.org/613893002/diff/1/courgette/adjustment_method_... > courgette/adjustment_method_2.cc:652: uint8 kind = 0; > On 2014/10/01 20:49:37, dgarrett wrote: > > Seems like Shingle::kWidth should also be updated to match. Is that awkward > > somehow? > > It just involves touching more code, but you're right, it probably should be > updated to prevent someone from making it >= UINT8_MAX in the future. Changed. > > https://codereview.chromium.org/613893002/diff/1/courgette/disassembler_win32... > File courgette/disassembler_win32_x64.cc (right): > > https://codereview.chromium.org/613893002/diff/1/courgette/disassembler_win32... > courgette/disassembler_win32_x64.cc:576: // !!! Is this cast safe? > On 2014/10/01 20:49:37, dgarrett wrote: > > I think that adding a comment describing the concerns is very > > reasonable. An error check to blow up if the assumption is wrong it also > > reasonable. > > OK. I implemented this by way of a checked_cast<>, which will CHECK if the > assumption is violated. Let me know if you meant for failure in this case to do > something less severe, like just returning false. > > https://codereview.chromium.org/613893002/diff/1/courgette/encoded_program.cc > File courgette/encoded_program.cc (right): > > https://codereview.chromium.org/613893002/diff/1/courgette/encoded_program.cc... > courgette/encoded_program.cc:569: current_rva += static_cast<RVA>(count); > On 2014/10/01 20:49:37, dgarrett wrote: > > I suspect that count should remain a uint32 here. That IS what's being read > out > > of the encoded format. > > ? |copy_counts_| is now a SizeTVector, so |count| really is a size_t. This goes > hand-in-hand with changing AddCopy() to take a size_t, etc. > > As I said previously, it would also be possible to undo all the size_t changes > (they're all connected) and use RVA everywhere, in which case the cast here > would basically move to DisassemblerElf32::ParseSimpleRegion(). We could then > decide to upgrade that cast to something like a checked_cast. > > (Or we could just upgrade the cast here.) And that means I made a mistake reading through the code earlier. The final patch encoding should not have any platform specific sizes in it. It should be possible to create a patch with a 64 bit binary on a 64 bit server, then apply it with a 32 bit client. Intermediate data is fair game, only the final patch encoding is what should be fixed across platforms.
On 2014/10/01 23:15:06, dgarrett wrote: > https://codereview.chromium.org/613893002/diff/1/courgette/encoded_program.cc... > > courgette/encoded_program.cc:569: current_rva += static_cast<RVA>(count); > > On 2014/10/01 20:49:37, dgarrett wrote: > > > I suspect that count should remain a uint32 here. That IS what's being read > > out > > > of the encoded format. > > > > ? |copy_counts_| is now a SizeTVector, so |count| really is a size_t. This > goes > > hand-in-hand with changing AddCopy() to take a size_t, etc. > > > > As I said previously, it would also be possible to undo all the size_t changes > > (they're all connected) and use RVA everywhere, in which case the cast here > > would basically move to DisassemblerElf32::ParseSimpleRegion(). We could then > > decide to upgrade that cast to something like a checked_cast. > > > > (Or we could just upgrade the cast here.) > > And that means I made a mistake reading through the code earlier. The final > patch encoding should not have any platform specific sizes in it. It should be > possible to create a patch with a 64 bit binary on a 64 bit server, then apply > it with a 32 bit client. > > Intermediate data is fair game, only the final patch encoding is what should be > fixed across platforms. Sorry, but can you be clearer as to what this reply means? Note that when we run copy_counts_ through WriteVector() and ReadVector(), they will write and read the underlying values as 32-bit quantities, and LOG_ASSERT() if this results in truncation. I think that guarantees what you want, but I don't understand the functionality well enough to be 100% sure. The alternatives I was suggesting had the following consequences: * Using uint32 everywhere would result in the truncation happening in DisassemblerElf32::ParseSimpleRegion() instead of WriteSizeVarint32(), so we could insert some kind of check there or even push the 32-bitness up more levels in the call chain (i.e. not pass size_ts to that function). * Using RVA everywhere would have the same consequences (since it's really a uint32), but might be more or less clear, or have maintenance consequences if that typedef could change in some way. * Adding some kind of check in EncodedProgram::AssembleTo() would be pointless given that ReadSizeVarint32() will only ever return a 32-bit quantity, so ignore that I even suggested that Basically, it comes down to whether, if ParseSimpleRegion() were to compute a >32-bit length, you want that to cause an immediate checkfailure or something, or whether you'd rather let the system handle it correctly until we try to write out the value, at which point we LOG_ASSERT. This change does the latter.
On 2014/10/01 23:30:12, Peter Kasting wrote: > On 2014/10/01 23:15:06, dgarrett wrote: > > > https://codereview.chromium.org/613893002/diff/1/courgette/encoded_program.cc... > > > courgette/encoded_program.cc:569: current_rva += static_cast<RVA>(count); > > > On 2014/10/01 20:49:37, dgarrett wrote: > > > > I suspect that count should remain a uint32 here. That IS what's being > read > > > out > > > > of the encoded format. > > > > > > ? |copy_counts_| is now a SizeTVector, so |count| really is a size_t. This > > goes > > > hand-in-hand with changing AddCopy() to take a size_t, etc. > > > > > > As I said previously, it would also be possible to undo all the size_t > changes > > > (they're all connected) and use RVA everywhere, in which case the cast here > > > would basically move to DisassemblerElf32::ParseSimpleRegion(). We could > then > > > decide to upgrade that cast to something like a checked_cast. > > > > > > (Or we could just upgrade the cast here.) > > > > And that means I made a mistake reading through the code earlier. The final > > patch encoding should not have any platform specific sizes in it. It should be > > possible to create a patch with a 64 bit binary on a 64 bit server, then apply > > it with a 32 bit client. > > > > Intermediate data is fair game, only the final patch encoding is what should > be > > fixed across platforms. > > Sorry, but can you be clearer as to what this reply means? > > Note that when we run copy_counts_ through WriteVector() and ReadVector(), they > will write and read the underlying values as 32-bit quantities, and LOG_ASSERT() > if this results in truncation. I think that guarantees what you want, but I > don't understand the functionality well enough to be 100% sure. > > The alternatives I was suggesting had the following consequences: > * Using uint32 everywhere would result in the truncation happening in > DisassemblerElf32::ParseSimpleRegion() instead of WriteSizeVarint32(), so we > could insert some kind of check there or even push the 32-bitness up more levels > in the call chain (i.e. not pass size_ts to that function). > * Using RVA everywhere would have the same consequences (since it's really a > uint32), but might be more or less clear, or have maintenance consequences if > that typedef could change in some way. > * Adding some kind of check in EncodedProgram::AssembleTo() would be pointless > given that ReadSizeVarint32() will only ever return a 32-bit quantity, so ignore > that I even suggested that > > Basically, it comes down to whether, if ParseSimpleRegion() were to compute a > >32-bit length, you want that to cause an immediate checkfailure or something, > or whether you'd rather let the system handle it correctly until we try to write > out the value, at which point we LOG_ASSERT. This change does the latter. As long as values > 32 bit error out before we write them to the final binary encoding, I'm happy. I'm just trying to make sure that a patch generated on a 64 bit server will always work on a 32 bit client, since we do that in the field today.
On 2014/10/02 22:00:17, dgarrett wrote: > As long as values > 32 bit error out before we write them to the final binary > encoding, I'm happy. I _think_ so, assuming my understanding of this code and of LOG_ASSERT are accurate. It may not be wise to rely on that. > I'm just trying to make sure that a patch generated on a 64 bit server will > always work on a 32 bit client, since we do that in the field today. Do the trybots verify this case? The gold standard here is running tests rather than just trying to interpret the source code by looking :)
On 2014/10/02 22:03:50, Peter Kasting wrote: > On 2014/10/02 22:00:17, dgarrett wrote: > > As long as values > 32 bit error out before we write them to the final binary > > encoding, I'm happy. > > I _think_ so, assuming my understanding of this code and of LOG_ASSERT are > accurate. It may not be wise to rely on that. > > > I'm just trying to make sure that a patch generated on a 64 bit server will > > always work on a 32 bit client, since we do that in the field today. > > Do the trybots verify this case? The gold standard here is running tests rather > than just trying to interpret the source code by looking :) They do test applying patches that already exist. That's not exactly the same thing, but is pretty good verification. I think this is good, but please feel free to add more test cases as well. LGTM
On 2014/10/02 22:12:32, dgarrett wrote: > I think this is good, but please feel free to add more test cases as well. I don't think I understand the system well enough to write some, unfortunately... landing.
The CQ bit was checked by pkasting@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/613893002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
FYI, in the latest patch set I removed some dead code in assembly_program.* that was calling printf() and thus triggering presubmit errors.
The CQ bit was checked by pkasting@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/613893002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
dgarrett: It looks like image_base() really isn't guaranteed to return a value that fits in 32 bits. The try failures here seem to indicate that the checked_cast<> I added to disassembler_win32_x64.cc is failing. I don't know what to do about this. Was the old code somehow operating correctly even though it was throwing away the upper 32 bits of the calculation?
On 2014/10/03 00:51:05, Peter Kasting wrote: > dgarrett: It looks like image_base() really isn't guaranteed to return a value > that fits in 32 bits. The try failures here seem to indicate that the > checked_cast<> I added to disassembler_win32_x64.cc is failing. > > I don't know what to do about this. Was the old code somehow operating > correctly even though it was throwing away the upper 32 bits of the calculation? Some of the values we generate can be wrong and we still get a valid (though much larger) patch. So, I suspect that's what was happening.
On 2014/10/03 00:55:01, dgarrett wrote: > On 2014/10/03 00:51:05, Peter Kasting wrote: > > dgarrett: It looks like image_base() really isn't guaranteed to return a value > > that fits in 32 bits. The try failures here seem to indicate that the > > checked_cast<> I added to disassembler_win32_x64.cc is failing. > > > > I don't know what to do about this. Was the old code somehow operating > > correctly even though it was throwing away the upper 32 bits of the > calculation? > > Some of the values we generate can be wrong and we still get a valid (though > much larger) patch. So, I suspect that's what was happening. So how should the code here deal with things? Change the checked_cast<> into a "if (doesn't fit in 32 bits) return false"? Just blindly cast and add some kind of comment explaining why it's OK (which I don't understand well enough to write)?
On 2014/10/03 01:03:46, Peter Kasting wrote: > On 2014/10/03 00:55:01, dgarrett wrote: > > On 2014/10/03 00:51:05, Peter Kasting wrote: > > > dgarrett: It looks like image_base() really isn't guaranteed to return a > value > > > that fits in 32 bits. The try failures here seem to indicate that the > > > checked_cast<> I added to disassembler_win32_x64.cc is failing. > > > > > > I don't know what to do about this. Was the old code somehow operating > > > correctly even though it was throwing away the upper 32 bits of the > > calculation? > > > > Some of the values we generate can be wrong and we still get a valid (though > > much larger) patch. So, I suspect that's what was happening. > > So how should the code here deal with things? Change the checked_cast<> into a > "if (doesn't fit in 32 bits) return false"? Just blindly cast and add some kind > of comment explaining why it's OK (which I don't understand well enough to > write)? I think we should file a bug back to the author of win64, then blindly cast with a comment that refers to the bug.
On 2014/10/03 01:09:11, dgarrett wrote: > On 2014/10/03 01:03:46, Peter Kasting wrote: > > On 2014/10/03 00:55:01, dgarrett wrote: > > > On 2014/10/03 00:51:05, Peter Kasting wrote: > > > > dgarrett: It looks like image_base() really isn't guaranteed to return a > > value > > > > that fits in 32 bits. The try failures here seem to indicate that the > > > > checked_cast<> I added to disassembler_win32_x64.cc is failing. > > > > > > > > I don't know what to do about this. Was the old code somehow operating > > > > correctly even though it was throwing away the upper 32 bits of the > > > calculation? > > > > > > Some of the values we generate can be wrong and we still get a valid (though > > > much larger) patch. So, I suspect that's what was happening. > > > > So how should the code here deal with things? Change the checked_cast<> into > a > > "if (doesn't fit in 32 bits) return false"? Just blindly cast and add some > kind > > of comment explaining why it's OK (which I don't understand well enough to > > write)? > > I think we should file a bug back to the author of win64, then blindly cast with > a comment that refers to the bug. I agree - this needs more investigation, the image_base can certainly be > 2^32 see crbug.com/361670 for why. Please do a simple cast and file a bug and assign it to me, and I will check this code. I am in the middle of adding some new 64-bit capabilities to courgette at the moment anyway, so I can look at it at the same time. Thanks.
On 2014/10/03 01:51:08, Will Harris wrote: > Please do a simple cast and file a bug and > assign it to me, and I will check this code. Filed http://crbug.com/419996 . I will make the code change and try to land this tomorrow. Thanks Will!
The CQ bit was checked by pkasting@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/613893002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 3cc302758932b7e53b68fe4a52eb20335e65f682
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8e3a26aaf2392d784ba6d5cbb696da926230a722 Cr-Commit-Position: refs/heads/master@{#298069} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
