1399777 - Fprinter should output to the Windows debug console when it outputs to stderr

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P5)

Description

[David Teller [:Yoric] - still alive but not very active]

In vm/Printer.*, we define several generic printers that are used among other things to display debugging information to stderr in DEBUG builds. Under Windows, stderr is not nearly as useful as the Windows Debugging console, so we should make sure that the data is duplicated there.

We can do this by patching `Fprinter::put` to:

(under Windows)

- check if `file_` is `stderr`;
- check if `IsDebuggerPresent()`;
- if so, copy the string and use `OutputDebugStringA` to pass it to the debugger.

Comment 1

[sourav.mukherjee619]

I want to work on this one.

Comment 2

[sourav.mukherjee619]

Attachment: Bug 1399777 Fprinter directing output to Windows debug console when it outputs to stderr.

Review commit: https://reviewboard.mozilla.org/r/180284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180284/

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8908618 [details]
Bug 1399777 Fprinter directing output to Windows debug console when it outputs to stderr.

https://reviewboard.mozilla.org/r/180284/#review185436

::: js/src/vm/Printer.cpp:453
(Diff revision 1)
>      int i = fwrite(s, /*size=*/ 1, /*nitems=*/ len, file_);
>      if (size_t(i) != len) {
>          reportOutOfMemory();
>          return false;
>      }
> +#ifdef WIN32

You could replace this with
```c++
#if defined(WIN32) && defined(DEBUG)
```

On the other hand, please use `XP_WIN32` instead of `WIN32`.

::: js/src/vm/Printer.cpp:455
(Diff revision 1)
>          reportOutOfMemory();
>          return false;
>      }
> +#ifdef WIN32
> +#ifdef DEBUG
> +    if(file_==stderr) {

Nit: `if (file_ == stderr) {` (with the whitespace).

::: js/src/vm/Printer.cpp:455
(Diff revision 1)
>          reportOutOfMemory();
>          return false;
>      }
> +#ifdef WIN32
> +#ifdef DEBUG
> +    if(file_==stderr) {

You also need to make sure that a debugger is present, as in the code you removed with your previous patch.

Use `IsDebuggerPresent()` for this.

::: js/src/vm/Printer.cpp:456
(Diff revision 1)
>          return false;
>      }
> +#ifdef WIN32
> +#ifdef DEBUG
> +    if(file_==stderr) {
> +        char *copy=js_strdup(s);

`OutputDebugStringA` expects the string to end with a `\0`, as is usual in C, but `put` doesn't guarantee that.

So you'd need to create an array of length `len + 1`, copy `len` bytes starting from `s`, then set the last byte to `0`.

::: js/src/vm/Printer.cpp:462
(Diff revision 1)
> +        OutputDebugStringA(copy);
> +        js_free(copy);
> +    }
> +#endif
> +#endif
>      return true;

Have you checked that it builds? You may need to include `<windows.h>`.

Comment 4

[sourav.mukherjee619]

Comment on attachment 8908618 [details]
Bug 1399777 Fprinter directing output to Windows debug console when it outputs to stderr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180284/diff/1-2/

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8908618 [details]
Bug 1399777 Fprinter directing output to Windows debug console when it outputs to stderr.

https://reviewboard.mozilla.org/r/180284/#review185538

::: js/src/vm/Printer.cpp:458
(Diff revisions 1 - 2)
>      if (size_t(i) != len) {
>          reportOutOfMemory();
>          return false;
>      }
> -#ifdef WIN32
> -#ifdef DEBUG
> +#ifdef XP_WIN32
> +    if((file_ == stderr) && (IsDebuggerPresent()) ) {

Looks good, but could you addspaces after `,`, around `=` and `+`?

Also, could you use `PodCopy` (defined here: http://searchfox.org/mozilla-central/source/mfbt/PodOperations.h#97 ) instead of `std::copy`?

Comment 6

[sourav.mukherjee619]

Comment on attachment 8908618 [details]
Bug 1399777 Fprinter directing output to Windows debug console when it outputs to stderr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180284/diff/2-3/

Comment 7

[sourav.mukherjee619]

Comment on attachment 8908618 [details]
Bug 1399777 Fprinter directing output to Windows debug console when it outputs to stderr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180284/diff/3-4/

Comment 8

[sourav.mukherjee619]

Some jobs failed in the try server. All of them are intermittent test failure.
Does it mean there is a problem which I have to solve ? Or is it fine ?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

I have difficulties relaunching stuff on Try. I *think* that the problem are not caused by your code, but I'd like try to confirm my intuition. I'll keep you updated.

Comment 10

[André Bargull [:anba]]

Comment on attachment 8908618 [details]
Bug 1399777 Fprinter directing output to Windows debug console when it outputs to stderr.

https://reviewboard.mozilla.org/r/180284/#review186458

::: js/src/vm/Printer.cpp:22
(Diff revision 4)
>  #include "jsutil.h"
>  
>  #include "ds/LifoAlloc.h"
>  
> +#ifdef XP_WIN32
> +#include <windows.h>

Please use jswin.h to include windows.h (http://searchfox.org/mozilla-central/source/js/src/jswin.h)

::: js/src/vm/Printer.cpp:458
(Diff revision 4)
>      if (size_t(i) != len) {
>          reportOutOfMemory();
>          return false;
>      }
> +#ifdef XP_WIN32
> +    if((file_ == stderr) && (IsDebuggerPresent()) ) {

Nit: extra whitespace and unneeded parentheses in `(IsDebuggerPresent()) )`.

::: js/src/vm/Printer.cpp:459
(Diff revision 4)
>          reportOutOfMemory();
>          return false;
>      }
> +#ifdef XP_WIN32
> +    if((file_ == stderr) && (IsDebuggerPresent()) ) {
> +        char *cpy = (char *) js_malloc(len + 1);

You need to handle the case when `js_malloc` returns nullptr on OOM. For example like in http://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/js/src/vm/Printer.cpp#131-135


Nit: SpiderMonkey style is to keep `*` next to the type, that means `char*` in the declaration and the cast.

::: js/src/vm/Printer.cpp:463
(Diff revision 4)
> +    if((file_ == stderr) && (IsDebuggerPresent()) ) {
> +        char *cpy = (char *) js_malloc(len + 1);
> +        PodCopy(cpy, s, len);
> +        cpy[len] = '\0';
> +        OutputDebugStringA(cpy);
> +        js_free(cpy);

Nit: You could use `UniqueChars buf(static_cast<char*>(js_malloc(len + 1)));`, that way you don't need to manually free the memory.

Comment 11

[André Bargull [:anba]]

(In reply to André Bargull from comment #10)
> Nit: You could use `UniqueChars buf(static_cast<char*>(js_malloc(len +
> 1)));`, that way you don't need to manually free the memory.

s/buf/cpy/

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Double-checked: try is now happy. Still, can you apply André's changes before we land this patch?

Comment 13

[sourav.mukherjee619]

Comment on attachment 8908618 [details]
Bug 1399777 Fprinter directing output to Windows debug console when it outputs to stderr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180284/diff/4-5/

Comment 14

[sourav.mukherjee619]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #12)
> Double-checked: try is now happy. Still, can you apply André's changes
> before we land this patch?

I have made the changes suggested and pushed them to the review.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8908618 [details]
Bug 1399777 Fprinter directing output to Windows debug console when it outputs to stderr.

https://reviewboard.mozilla.org/r/180284/#review187428

Looks good to me. Can you fix that last missing whitespace while I'm running it through our try server?

::: js/src/vm/Printer.cpp:460
(Diff revision 5)
>          return false;
>      }
> +#ifdef XP_WIN32
> +    if((file_ == stderr) && (IsDebuggerPresent())) {
> +        UniqueChars buf(static_cast<char*>(js_malloc(len + 1)));
> +        if(!buf) {

Nit: `if (` (missing whitespace).

Comment 16

[sourav.mukherjee619]

Comment on attachment 8908618 [details]
Bug 1399777 Fprinter directing output to Windows debug console when it outputs to stderr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180284/diff/5-6/

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8908618 [details]
Bug 1399777 Fprinter directing output to Windows debug console when it outputs to stderr.

https://reviewboard.mozilla.org/r/180284/#review187502

Ah, a few more things before we can land this:

1/ your commit message contains too many `MozReview-Commit-ID`. Could you edit it to keep only the first one? Otherwise, I don't think AutoLander will let me land this;

2/ in reviewboard, could you mark as fixed all the issues that you have fixed?

Comment 18

[sourav.mukherjee619]

Comment on attachment 8908618 [details]
Bug 1399777 Fprinter directing output to Windows debug console when it outputs to stderr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180284/diff/6-7/

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Note: there is still one issue marked as unfixed.

Comment 20

[sourav.mukherjee619]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #19)
> Note: there is still one issue marked as unfixed.

Sorry for missing it. Fixed it.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83afa692da84
Fprinter directing output to Windows debug console when it outputs to stderr. r=Yoric

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/83afa692da84

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Thanks for fixing this bug, sourav. Tell me if you need help finding another bug :)

Comment 24

[sourav.mukherjee619]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #23)
> Thanks for fixing this bug, sourav. Tell me if you need help finding another
> bug :)
Thanks for mentoring me.
I need help finding another bug.

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Would you be interested in bug 1403034? It's part of a larger set of cleanups we're doing in the code to avoid errors.

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Ah, it's already taken, but other similar bugs of the same cleanup (bug 1277133) are still available.

Comment 27

[sourav.mukherjee619]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #25)
> Would you be interested in bug 1403034? It's part of a larger set of
> cleanups we're doing in the code to avoid errors.

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #26)
> Ah, it's already taken, but other similar bugs of the same cleanup (bug
> 1277133) are still available.

Both the bugs are taken by someone else. Can you find me another bug ?