If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

Status

()

Core
JavaScript Engine
P5
normal
ASSIGNED
5 days ago
6 hours ago

People

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

Tracking

Firefox Tracking Flags

(Not tracked)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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
(Assignee)

Comment 1

5 days ago
I want to work on this one.
Assignee: nobody → sourav.mukherjee619
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Reporter)

Comment 3

4 days ago
mozreview-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/#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 hidden (mozreview-request)
(Reporter)

Comment 5

4 days ago
mozreview-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/#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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 days ago
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 10

8 hours ago
mozreview-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/#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-

Comment 11

8 hours ago
(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)
You need to log in before you can comment on or make changes to this bug.