Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10399043:
zygote: Redirect 64-bit libc localtime and localtime_r routines. (Closed)
|
Created:
8 years, 7 months ago by hshi1 Modified:
8 years, 7 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jochen+watch-content_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionzygote: Redirect 64-bit libc localtime and localtime_r routines.
BUG=128053
TEST=locally tested on lumpy
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137560
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address Jorge's comments. #
Total comments: 1
Patch Set 3 : Address Satoru's comments. #Patch Set 4 : Failure to locate 64-bit counterparts should not be Fatal. #Messages
Total messages: 21 (0 generated)
Jorge, Satoru - please review this fix. We need to redirect the 64-bit version of libc localtime() and localtime_r() functions. After this the local time is correct.
Add @sergeyu -- I noticed comments that the InitLibcFileIOFunctions() code does not work properly under ASAN. Would this problem affect localtime? Thanks.
LGTM, but you might want to get LGTM from another reviewer who's more familiar with the code.
http://codereview.chromium.org/10399043/diff/1/content/zygote/zygote_main_lin... File content/zygote/zygote_main_linux.cc (left): http://codereview.chromium.org/10399043/diff/1/content/zygote/zygote_main_lin... content/zygote/zygote_main_linux.cc:190: if (!g_libc_localtime || !g_libc_localtime_r) { Shouldn't you add checks for the *64 versions of the functions, since you're calling them in the new code? Are we sure the bug below does not apply in this case? http://codereview.chromium.org/10399043/diff/1/content/zygote/zygote_main_lin... File content/zygote/zygote_main_linux.cc (right): http://codereview.chromium.org/10399043/diff/1/content/zygote/zygote_main_lin... content/zygote/zygote_main_linux.cc:214: __attribute__ ((__visibility__("default"))) Why do we need the visibility annotations?
http://codereview.chromium.org/10399043/diff/1/content/zygote/zygote_main_lin... File content/zygote/zygote_main_linux.cc (right): http://codereview.chromium.org/10399043/diff/1/content/zygote/zygote_main_lin... content/zygote/zygote_main_linux.cc:214: __attribute__ ((__visibility__("default"))) This is a copy&paste based on the fopen/fopen64 code. I'm not sure if it is really needed or not... On 2012/05/16 02:59:45, Jorge Lucangeli Obes wrote: > Why do we need the visibility annotations?
Jorge - can you review Patch Set #2. Thanks. http://codereview.chromium.org/10399043/diff/1/content/zygote/zygote_main_lin... File content/zygote/zygote_main_linux.cc (left): http://codereview.chromium.org/10399043/diff/1/content/zygote/zygote_main_lin... content/zygote/zygote_main_linux.cc:190: if (!g_libc_localtime || !g_libc_localtime_r) { On 2012/05/16 02:59:45, Jorge Lucangeli Obes wrote: > Shouldn't you add checks for the *64 versions of the functions, since you're > calling them in the new code? Are we sure the bug below does not apply in this > case? Done. http://codereview.chromium.org/10399043/diff/1/content/zygote/zygote_main_lin... File content/zygote/zygote_main_linux.cc (right): http://codereview.chromium.org/10399043/diff/1/content/zygote/zygote_main_lin... content/zygote/zygote_main_linux.cc:214: __attribute__ ((__visibility__("default"))) It appears to be useless. I would assume that visiblity is "default" by default! On 2012/05/16 03:04:09, hshi1 wrote: > This is a copy&paste based on the fopen/fopen64 code. I'm not sure if it is > really needed or not... > > On 2012/05/16 02:59:45, Jorge Lucangeli Obes wrote: > > Why do we need the visibility annotations? >
http://codereview.chromium.org/10399043/diff/1/content/zygote/zygote_main_lin... File content/zygote/zygote_main_linux.cc (right): http://codereview.chromium.org/10399043/diff/1/content/zygote/zygote_main_lin... content/zygote/zygote_main_linux.cc:214: __attribute__ ((__visibility__("default"))) Sorry, I take this back. It appears that the visibility setting is indeed necessary. Without this line I will get a system hang at boot time. I am guess that is why this is used in the tricks for fopen and fopen64 below.
On 2012/05/16 17:44:57, hshi1 wrote: > http://codereview.chromium.org/10399043/diff/1/content/zygote/zygote_main_lin... > File content/zygote/zygote_main_linux.cc (right): > > http://codereview.chromium.org/10399043/diff/1/content/zygote/zygote_main_lin... > content/zygote/zygote_main_linux.cc:214: __attribute__ > ((__visibility__("default"))) > Sorry, I take this back. It appears that the visibility setting is indeed > necessary. Without this line I will get a system hang at boot time. I am guess > that is why this is used in the tricks for fopen and fopen64 below. visibility is set to hidden by default in chrome per build/common.gypi: # Don't export any symbols (for example, to plugins we dlopen()). # Note: this is *required* to make some plugins work. '-fvisibility=hidden',
On 2012/05/16 17:51:04, satorux1 wrote: > On 2012/05/16 17:44:57, hshi1 wrote: > > > http://codereview.chromium.org/10399043/diff/1/content/zygote/zygote_main_lin... > > File content/zygote/zygote_main_linux.cc (right): > > > > > http://codereview.chromium.org/10399043/diff/1/content/zygote/zygote_main_lin... > > content/zygote/zygote_main_linux.cc:214: __attribute__ > > ((__visibility__("default"))) > > Sorry, I take this back. It appears that the visibility setting is indeed > > necessary. Without this line I will get a system hang at boot time. I am guess > > that is why this is used in the tricks for fopen and fopen64 below. > > visibility is set to hidden by default in chrome per build/common.gypi: > > # Don't export any symbols (for example, to plugins we dlopen()). > # Note: this is *required* to make some plugins work. > '-fvisibility=hidden', You might want to add some comment about this at __visibility__("default"), so others won't try removing it. :)
http://codereview.chromium.org/10399043/diff/13001/content/zygote/zygote_main... File content/zygote/zygote_main_linux.cc (left): http://codereview.chromium.org/10399043/diff/13001/content/zygote/zygote_main... content/zygote/zygote_main_linux.cc:222: struct tm* localtime_r(const time_t* timep, struct tm* result) { I wonder how it was working before if we weren't setting default visibility.
Jorge, it was working before (in the 32-bit era) because we did not need to rename the function to Xxxx_override. See comments in the fopen/fopen64 -- the reason why we define fopen_override with asm name fopen is so that it is not subject to the redirection to fopen64.
Uploaded Patch Set #3.
Adding brettw@ for review. Sorry I didn't realize earlier that contents/OWNER has noparent set so we need an owner. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@chromium.org/10399043/14005
Presubmit check for 10399043-14005 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
content
Presubmit checks took 1.9s to calculate.
On 2012/05/16 19:49:07, hshi1 wrote: hshi, since satorux suggested getting another LGTM, it might have been a good idea to have waited before kicking the CQ. lgtm
Thanks Jorge! Still awaiting review from one of the owners.
rubberstamp lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@chromium.org/10399043/14005
Change committed as 137560 |
