|
|
Created:
6 years, 8 months ago by scottmg Modified:
6 years, 8 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix --enable-logging after VS2013 switch
After https://src.chromium.org/viewvc/chrome?view=rev&revision=178656
a valid fileno of stdout or stderr abort connection of the console to
stdio on Windows.
Looking at the VS CRT between 2010 and 2013, it seems the behavior
that r178656 was relying on changed:
(in __initstdio)
C:\Program Files (x86)>diff "Microsoft Visual Studio 10.0\vc\crt\src\_file.c" "Microsoft Visual Studio 12.0\vc\crt\src\_file.c"
144,157d143
< for ( i = 0 ; i < 3 ; i++ ) {
< if ( (_osfhnd(i) == (intptr_t)INVALID_HANDLE_VALUE) ||
< (_osfhnd(i) == _NO_CONSOLE_FILENO) ||
< (_osfhnd(i) == 0) )
< {
< /*
< * For stdin, stdout & stderr, we use _NO_CONSOLE_FILENO (a value
< * different from _INVALID_HANDLE_VALUE to distinguish between
< * a failure in opening a file & a program run without a console.
< */
< _iob[i]._file = _NO_CONSOLE_FILENO;
< }
< }
<
194a181
> __piob = NULL;
There's an open bug
http://connect.microsoft.com/VisualStudio/feedback/details/785119/
but as it was broken in 2012 and not fixed in 2013, it doesn't look like it's
going to be fixed. (The documentation still suggests that r178656 is correct:
http://msdn.microsoft.com/en-us/library/zs6wbdhx(v=vs.100).aspx vs
http://msdn.microsoft.com/en-us/library/zs6wbdhx(v=vs.120).aspx ) but that's
little consolation.
So, confirm that the underlying HANDLE is valid before aborting.
TBR=brettw@chromium.org
R=mseaborn@chromium.org
BUG=169941, 309197, 358267
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260738
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261253
Patch Set 1 #
Total comments: 2
Patch Set 2 : . #Patch Set 3 : reland without xp #Messages
Total messages: 28 (0 generated)
Hi Mark, I'm a bit unclear on the specifics of what your previous patch was addressing, but does this seem reasonable?
The change looks OK. Did you check that Chrome still produces logging when run under Cygwin's terminal (with and without "--enable-logging")? If you don't have Cygwin, it should be equivalent to check whether: chrome --enable-logging >log 2>&1 sends logging to the 'log' file. https://codereview.chromium.org/219813003/diff/1/base/process/launch_win.cc File base/process/launch_win.cc (right): https://codereview.chromium.org/219813003/diff/1/base/process/launch_win.cc#n... base/process/launch_win.cc:70: intptr_t stdout_handle = _get_osfhandle(_fileno(stdout)); Does intptr_t work on the MSVC toolchains that Chrome supports? (I don't see trybot runs for this change. :-) ) stdint.h used not to be available in MSVC.
Thanks. On 2014/03/31 18:39:08, Mark Seaborn wrote: > The change looks OK. Did you check that Chrome still produces logging when run > under Cygwin's terminal (with and without "--enable-logging")? > > If you don't have Cygwin, it should be equivalent to check whether: > chrome --enable-logging >log 2>&1 > sends logging to the 'log' file. Yes, that seems to work OK with and without --enable-logging. > > https://codereview.chromium.org/219813003/diff/1/base/process/launch_win.cc > File base/process/launch_win.cc (right): > > https://codereview.chromium.org/219813003/diff/1/base/process/launch_win.cc#n... > base/process/launch_win.cc:70: intptr_t stdout_handle = > _get_osfhandle(_fileno(stdout)); > Does intptr_t work on the MSVC toolchains that Chrome supports? (I don't see > trybot runs for this change. :-) ) stdint.h used not to be available in MSVC.
https://codereview.chromium.org/219813003/diff/1/base/process/launch_win.cc File base/process/launch_win.cc (right): https://codereview.chromium.org/219813003/diff/1/base/process/launch_win.cc#n... base/process/launch_win.cc:70: intptr_t stdout_handle = _get_osfhandle(_fileno(stdout)); On 2014/03/31 18:39:08, Mark Seaborn wrote: > Does intptr_t work on the MSVC toolchains that Chrome supports? (I don't see > trybot runs for this change. :-) ) stdint.h used not to be available in MSVC. Assuming it works for the various compilers of base that aren't MSVC, it should be ok, we only build with 2013 now. trybots started now anyway. :)
+brettw
On 31 March 2014 11:46, <scottmg@chromium.org> wrote: > https://codereview.chromium.org/219813003/diff/1/base/ > process/launch_win.cc > File base/process/launch_win.cc (right): > > https://codereview.chromium.org/219813003/diff/1/base/ > process/launch_win.cc#newcode70 > base/process/launch_win.cc:70: intptr_t stdout_handle = > _get_osfhandle(_fileno(stdout)); > On 2014/03/31 18:39:08, Mark Seaborn wrote: > >> Does intptr_t work on the MSVC toolchains that Chrome supports? (I >> > don't see > >> trybot runs for this change. :-) ) stdint.h used not to be available >> > in MSVC. > > Assuming it works for the various compilers of base that aren't MSVC, it > should be ok, we only build with 2013 now. trybots started now anyway. > :) OK, LGTM, thanks. Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
tbr Brett
The CQ bit was checked by scottmg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/219813003/20001
Message was sent while issue was closed.
Change committed as 260738
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/220653002/ by tkent@chromium.org. The reason for reverting is: Speculative revert for massive failures of XP browser_tests. http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/builds... .
Message was sent while issue was closed.
On 2014/04/01 07:23:37, tkent wrote: > A revert of this CL has been created in > https://codereview.chromium.org/220653002/ by mailto:tkent@chromium.org. > > The reason for reverting is: Speculative revert for massive failures of XP > browser_tests. > > http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/builds... > . Also, this CL broke nacl_integration test, and the revert fixed the failure. http://build.chromium.org/p/chromium.win/builders/NaCl%20Tests%20%28x86-64%29...
Message was sent while issue was closed.
On 2014/04/01 07:23:37, tkent wrote: > A revert of this CL has been created in > https://codereview.chromium.org/220653002/ by mailto:tkent@chromium.org. > > The reason for reverting is: Speculative revert for massive failures of XP > browser_tests. > > http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/builds... > . Conformed this CL was the culprit of XP browser_tests failures.
Thanks for reverting, it looks NaCl-related in the browser_tests run too. On Tue, Apr 1, 2014 at 12:23 AM, <tkent@chromium.org> wrote: > A revert of this CL has been created in > https://codereview.chromium.org/220653002/ by tkent@chromium.org. > > The reason for reverting is: Speculative revert for massive failures of XP > browser_tests. > > http://build.chromium.org/p/chromium.win/builders/XP% > 20Tests%20%281%29/builds/30725 > . > > https://codereview.chromium.org/219813003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
After poking at this most of the day, I'm just going to try relanding without running this code on XP. I have no idea why it's problematic; details at https://code.google.com/p/chromium/issues/detail?id=358267#c4 .
The CQ bit was checked by scottmg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/219813003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Internal error: failed to checkout. Please try again.
The CQ bit was checked by scottmg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/219813003/60001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/219813003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel tryserver.chromium on linux_chromium_rel
The CQ bit was checked by scottmg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/219813003/60001
Message was sent while issue was closed.
Change committed as 261253
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/223353002/ by scottmg@chromium.org. The reason for reverting is: Possibly causing failures on nacl_integration x86-64 http://build.chromium.org/p/chromium.win/builders/NaCl%20Tests%20%28x86-64%29.... |