Closed Bug 1399777 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, enhancement, P5)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: Yoric, Assigned: sourav.mukherjee619, Mentored)

Details

(Whiteboard: [good first bug][c++])

Attachments

(1 file)

Bug 1399777 Fprinter directing output to Windows debug console when it outputs to stderr.
59 bytes, text/x-review-board-request
Yoric
: review+
Details 
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.
Whiteboard: [good first bug][c++]
Priority: -- → P5
I want to work on this one.
Assignee: nobody → sourav.mukherjee619
Status: NEW → ASSIGNED
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 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`?
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 ?
Flags: needinfo?(dteller)
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.
Flags: needinfo?(dteller)
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.
Attachment #8908618 - Flags: review-
(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/
Double-checked: try is now happy. Still, can you apply André's changes before we land this patch?
Flags: needinfo?(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.
Flags: needinfo?(sourav.mukherjee619)
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).
Attachment #8908618 - Flags: review?(dteller) → review+
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?
Note: there is still one issue marked as unfixed.
(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.
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/83afa692da84
Fprinter directing output to Windows debug console when it outputs to stderr. r=Yoric
https://hg.mozilla.org/mozilla-central/rev/83afa692da84
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Thanks for fixing this bug, sourav. Tell me if you need help finding another bug :)
Flags: needinfo?(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.
Flags: needinfo?(sourav.mukherjee619)
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.
Flags: needinfo?(sourav.mukherjee619)
Ah, it's already taken, but other similar bugs of the same cleanup (bug 1277133) are still available.
(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 ?
Flags: needinfo?(sourav.mukherjee619)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: