Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
310323003:
Fix SafeSPrintfTest.Truncation in base_unittests (Closed)
|
Created:
6 years, 6 months ago by Yang Gu Modified:
6 years, 6 months ago Reviewers:
Markus (顧孟勤), Mark Mentovai, Nico, Inactive, Markus Gutschke (顧孟勤), darin (slow to review) CC:
chromium-reviews, erikwright+watch_chromium.org, jshin+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFix SafeSPrintfTest.Truncation in base_unittests
On 32-bit platform, the expected result for this test is address of
64-bit, while the actual result is address of 32-bit. So they don't
match with each other.
BUG=380492
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277986
Patch Set 1 #
Total comments: 2
Patch Set 2 : Change to uintptr_t #
Total comments: 9
Patch Set 3 : Remove inttypes.h #
Total comments: 3
Patch Set 4 : Use c++ cast style and fix typo in comment #Messages
Total messages: 17 (0 generated)
Please review this!
https://codereview.chromium.org/310323003/diff/1/base/strings/safe_sprintf_un... File base/strings/safe_sprintf_unittest.cc (right): https://codereview.chromium.org/310323003/diff/1/base/strings/safe_sprintf_un... base/strings/safe_sprintf_unittest.cc:430: (intptr_t)PrintLongString); uintptr_t ? (http://www.nongnu.org/avr-libc/user-manual/group__avr__inttypes.html#ga9c3c25...)
https://codereview.chromium.org/310323003/diff/1/base/strings/safe_sprintf_un... File base/strings/safe_sprintf_unittest.cc (right): https://codereview.chromium.org/310323003/diff/1/base/strings/safe_sprintf_un... base/strings/safe_sprintf_unittest.cc:430: (intptr_t)PrintLongString); Thanks to point this out. uintptr_t is more meaningful for function pointer. CL is changed.
Seems fine but I am not an OWNER for this area :)
On 2014/06/05 02:03:34, Chris Dumez wrote: > Seems fine but I am not an OWNER for this area :) Thank you very much for comments! I will wait for feedback from others:)
@markus, could you please review this patch?
Couldn't find @Markus (顧孟勤), anyone else can take a look at this patch? Thanks!
https://codereview.chromium.org/310323003/diff/20001/base/strings/safe_sprint... File base/strings/safe_sprintf_unittest.cc (right): https://codereview.chromium.org/310323003/diff/20001/base/strings/safe_sprint... base/strings/safe_sprintf_unittest.cc:420: // string for sprintf() is nor complicated, as it does not have the This should say “not”, not “nor”. https://codereview.chromium.org/310323003/diff/20001/base/strings/safe_sprint... base/strings/safe_sprintf_unittest.cc:428: sprintf(ref, "A long string: %%d 00DEADBEEF %lld 0x%" PRIXPTR " <NULL>", Why can’t you use %p here? https://codereview.chromium.org/310323003/diff/20001/base/strings/safe_sprint... base/strings/safe_sprintf_unittest.cc:429: static_cast<long long>(std::numeric_limits<intptr_t>::min()), Wouldn’t PRIdPTR (and no cast) be better here and in the format strings used above on lines 379 and 381?
https://codereview.chromium.org/310323003/diff/20001/base/strings/safe_sprint... File base/strings/safe_sprintf_unittest.cc (right): https://codereview.chromium.org/310323003/diff/20001/base/strings/safe_sprint... base/strings/safe_sprintf_unittest.cc:429: static_cast<long long>(std::numeric_limits<intptr_t>::min()), Mark Mentovai wrote: > Wouldn’t PRIdPTR (and no cast) be better here and in the format strings used > above on lines 379 and 381? Never mind, I see that %d is intentionally used above, in the comment on line 370.
https://codereview.chromium.org/310323003/diff/20001/base/strings/safe_sprint... File base/strings/safe_sprintf_unittest.cc (right): https://codereview.chromium.org/310323003/diff/20001/base/strings/safe_sprint... base/strings/safe_sprintf_unittest.cc:7: #include <inttypes.h> Does MSVC even have this header? https://codereview.chromium.org/310323003/diff/20001/base/strings/safe_sprint... base/strings/safe_sprintf_unittest.cc:429: static_cast<long long>(std::numeric_limits<intptr_t>::min()), Mark Mentovai wrote: > Mark Mentovai wrote: > > Wouldn’t PRIdPTR (and no cast) be better here and in the format strings used > > above on lines 379 and 381? > > Never mind, I see that %d is intentionally used above, in the comment on line > 370. The “never mind” applies to lines 379 and 381. I still think PRIdPTR would be better down here.
https://codereview.chromium.org/310323003/diff/20001/base/strings/safe_sprint... File base/strings/safe_sprintf_unittest.cc (right): https://codereview.chromium.org/310323003/diff/20001/base/strings/safe_sprint... base/strings/safe_sprintf_unittest.cc:7: #include <inttypes.h> On 2014/06/15 12:27:26, Mark Mentovai wrote: > Does MSVC even have this header? Not quite sure about MSVC. I searched the web and seems it might not contain this header. I see some other places in this file use "%llX" (switch back to original format) as format and cast pointer to "(unsigned long long)(uintptr_t)". Looks ok according to my test on 32-bit and 64-bit. https://codereview.chromium.org/310323003/diff/20001/base/strings/safe_sprint... base/strings/safe_sprintf_unittest.cc:428: sprintf(ref, "A long string: %%d 00DEADBEEF %lld 0x%" PRIXPTR " <NULL>", On 2014/06/15 12:19:21, Mark Mentovai wrote: > Why can’t you use %p here? The actual format is in SafeSNPrintf(), where %p is formatted as upper case. But the expected result is in sprintf, where %p is in lower case. https://codereview.chromium.org/310323003/diff/20001/base/strings/safe_sprint... base/strings/safe_sprintf_unittest.cc:429: static_cast<long long>(std::numeric_limits<intptr_t>::min()), On 2014/06/15 12:27:26, Mark Mentovai wrote: > Mark Mentovai wrote: > > Mark Mentovai wrote: > > > Wouldn’t PRIdPTR (and no cast) be better here and in the format strings used > > > above on lines 379 and 381? > > > > Never mind, I see that %d is intentionally used above, in the comment on line > > 370. > > The “never mind” applies to lines 379 and 381. I still think PRIdPTR would be > better down here. PRIdPTR is in decimal format, but we actually need hexadecimal format (%p).
https://codereview.chromium.org/310323003/diff/40001/base/strings/safe_sprint... File base/strings/safe_sprintf_unittest.cc (right): https://codereview.chromium.org/310323003/diff/40001/base/strings/safe_sprint... base/strings/safe_sprintf_unittest.cc:419: // string for sprintf() is nor complicated, as it does not have the Please fix this comment as requested: nor → not. https://codereview.chromium.org/310323003/diff/40001/base/strings/safe_sprint... base/strings/safe_sprintf_unittest.cc:428: static_cast<long long>(std::numeric_limits<intptr_t>::min()), When I mentioned using PRIdPTR, I was talking about this argument, which is currently formatted with %lld. You seemed to misunderstand that I was talking about the next one, which is formatted with %llX. However, since we’ve decided not to use <inttypes.h>, the point is moot as we won’t be using any of the PRI* macros from that header. https://codereview.chromium.org/310323003/diff/40001/base/strings/safe_sprint... base/strings/safe_sprintf_unittest.cc:429: (unsigned long long)(uintptr_t)PrintLongString); 1. What’s with the double cast now? 2. Don’t use (C-style)casts. Use c++<style>(casts). This one should be a reinterpret_cast. The C++ style guide covers this.
Let me try to clarify the whole thing. First, the issue occurs to the expected output of "PrintLongString". So I didn't expect you were talking about the previous one. The actual result (SafeSNPrintf) uses %p, and it would format the output to be upper case. I think we need to modify the expected output (sprintf) so that they can match with each other. As the expected output is in upper case, we can't use %p in sprintf (It will be lower case), so we have to use %X. Below are some experimental results using different formats (format, PrintLongString with necessary cast): 1. %lX, (uintptr_t)PrintLongString This works for Linux gcc, both 32-bit and 64-bit, but not for MSVC 64-bit. The latter would deem "long int" as 32-bit even on 64-bit platform. 2. %llX, (unsigned long long)PrintLongString Since above reason, we have to use %llX to support all platforms (By the way, this is not efficient for 32-bit). But if we just directly cast it to "unsigned long long", we'd meet another problem about sign extension (When a negative signed integer is promoted to an unsigned integer of larger type, it is first promoted to the signed equivalent of the larger type, then converted to the unsigned value). For example, below are error messages on my 32-bit platform: Value of: std::string(tmp.get()) Actual: "A long string: %d 00DEADBEEF -2147483648 0xE9CD137F <NULL>" Expected: std::string(ref).substr(0, kSSizeMax-1) Which is: "A long string: %d 00DEADBEEF -2147483648 0xFFFFFFFFE9CD137F <NULL>" ../../../base/strings/safe_sprintf_unittest.cc:439: Failure 3. %llX, (unsigned long long)(uintptr_t)PrintLongString So I have to cast it to uintptr_t first to solve the sign extension issue. By the way, this kind of C style cast happens many other places in this file, such as: line 330: sprintf(addr, "0x%llX", (unsigned long long)(uintptr_t)buf); line 335: sprintf(addr, "0x%llX", (unsigned long long)(uintptr_t)sprintf); 4. %llX, static_cast<unsigned long long>(reinterpret_cast<uintptr_t>(PrintLongString))) This is the c++ version of above solution. Following your advice, I will update the CL as this. Thanks a lot for your valuable comments!
Gotcha. Thanks for the description of the process. LGTM.
The CQ bit was checked by yang.gu@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yang.gu@intel.com/310323003/80001
Message was sent while issue was closed.
Change committed as 277986 |
