TraceLogger: write files to $TLDIR

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
My /tmp directory keeps getting cluttered with multiple batches of tracelogger outputs, and it's hard to keep straight which is which. It would be nice to be able to pick an output directory.
(Assignee)

Comment 1

3 years ago
Created attachment 8783212 [details] [diff] [review]
TraceLogger: write files to $TLDIR
Attachment #8783212 - Flags: review?(hv1989)
(Assignee)

Comment 2

3 years ago
Created attachment 8783213 [details] [diff] [review]
TraceLogger: write files to $TLDIR

Oops, one hunk was in the wrong patch.
Attachment #8783213 - Flags: review?(hv1989)
(Assignee)

Updated

3 years ago
Attachment #8783212 - Attachment is obsolete: true
Attachment #8783212 - Flags: review?(hv1989)
Comment on attachment 8783213 [details] [diff] [review]
TraceLogger: write files to $TLDIR

Review of attachment 8783213 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! Could you also document this on https://github.com/h4writer/tracelogger ?

::: js/src/vm/TraceLoggingGraph.cpp
@@ +54,5 @@
> +            p++;
> +            if (*p == 'u')
> +                len += sizeof("4294967295") - 1;
> +            else if (*p == 'd')
> +                len += sizeof("999") - 1;

Can we add asserts that check this? If possible that might be good to make sure %d doesn't get confused as being any int32 value.
Attachment #8783213 - Flags: review?(hv1989) → review+
(Assignee)

Comment 4

3 years ago
(In reply to Hannes Verschore [:h4writer] from comment #3)
> Comment on attachment 8783213 [details] [diff] [review]
> TraceLogger: write files to $TLDIR
> 
> Review of attachment 8783213 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice! Could you also document this on
> https://github.com/h4writer/tracelogger ?

Yeah, I'll send you a PR for this and for displaying the thread names.

> ::: js/src/vm/TraceLoggingGraph.cpp
> @@ +54,5 @@
> > +            p++;
> > +            if (*p == 'u')
> > +                len += sizeof("4294967295") - 1;
> > +            else if (*p == 'd')
> > +                len += sizeof("999") - 1;
> 
> Can we add asserts that check this? If possible that might be good to make
> sure %d doesn't get confused as being any int32 value.

I don't know how, because the actual values are passed through varargs. At least without reimplementing a printf format string parser.

Oh! I guess I could at least check whether the final vsnprintf'ed string ended up too long. "Guess what? A second ago, you just trashed some random memory!"

Comment 5

3 years ago
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fe209e9ca79
TraceLogger: write files to $TLDIR, r=h4writer
Created attachment 8784411 [details] [diff] [review]
Few fixes

Some small fixes I didn't see while reviewing.
Attachment #8784411 - Flags: review?(sphink)
(Assignee)

Comment 7

3 years ago
Comment on attachment 8784411 [details] [diff] [review]
Few fixes

Review of attachment 8784411 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I tried actually using it yesterday, and ended up with the same fixes. If it's ok, I'll just land my current patch, which already incorporates what you have here.

My TL-related patch stack is waiting on a review in bug 1296878, though.
Attachment #8784411 - Flags: review?(sphink)
(In reply to Steve Fink [:sfink] [:s:] from comment #7)
> Comment on attachment 8784411 [details] [diff] [review]
> Few fixes
> 
> Review of attachment 8784411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah, I tried actually using it yesterday, and ended up with the same fixes.
> If it's ok, I'll just land my current patch, which already incorporates what
> you have here.
> 
> My TL-related patch stack is waiting on a review in bug 1296878, though.

Sure

Comment 9

3 years ago
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c12dbf24cac
TraceLogger: write files to $TLDIR, r=h4writer

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1c12dbf24cac
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.