This is an archive of the discontinued LLVM Phabricator instance.

[XRay] [compiler-rt] Include argv[0] in the log file name.
ClosedPublic

Authored by pelikan on Dec 18 2016, 10:12 PM.

Details

Summary

If you decide to recompile parts of your Linux distro with XRay, it may
be useful to know which trace belongs to which binary. While there, get
rid of the incorrect strncat() usage; it always returns a pointer to the
start which makes that if() always true. Replace with snprintf which is
bounded so that enough from both strings fits nicely.

Event Timeline

pelikan updated this revision to Diff 81915.Dec 18 2016, 10:12 PM
pelikan retitled this revision from to [XRay] [compiler-rt] Include argv[0] in the log file name..
pelikan updated this object.
pelikan added a reviewer: dberris.
pelikan added a subscriber: llvm-commits.
dberris requested changes to this revision.Dec 18 2016, 10:19 PM
dberris edited edge metadata.
dberris added inline comments.
lib/xray/xray_inmemory_log.cc
116

How about instead of "UNKNOWN_PROGNAME" just 'unknown'?

121

No braces for one-liners?

124–125

What's the 120?

127

Is there a better name than "Need"? How about Overflow?

This revision now requires changes to proceed.Dec 18 2016, 10:19 PM
pelikan updated this revision to Diff 81916.Dec 18 2016, 10:27 PM
pelikan edited edge metadata.
pelikan marked 2 inline comments as done.

address comments

pelikan marked an inline comment as done.Dec 18 2016, 10:28 PM

Mark as done.

lib/xray/xray_inmemory_log.cc
116

I was actually thinking of "(unknown)", similar to when printf(3) prints "(null)" on NULL strings. I don't have a strong preference but wanted something that immediately suggests something's gone wrong.

121

I tried to find it in the coding standards, I promise! :-)

124–125

The buffer is 256 characters long; 120 is a decent bound for not overflowing the string. Should I bind it with sizeof(TmpFilename) somehow? Its size is a magic number too, and they're close to one another for people to see.

127

The function returns how many bytes would you need to satisfy the operation; "overflow" would indicate it's the amount of characters over 256 (sizeof TmpFilename) IMHO. Do you still want me to change it?

dberris requested changes to this revision.Dec 18 2016, 11:33 PM
dberris edited edge metadata.
dberris added inline comments.
lib/xray/xray_inmemory_log.cc
116

"(unknown)" seems strictly better if not annoying to refer to in the filesystem. Either that or just keeping it empty?

124–125

Yeah, you can probably do something about making it relative to the sizeof(TmpFilename).

127

Yeah, so "Need" isn't the best name. Is "Length" a better name?

This revision now requires changes to proceed.Dec 18 2016, 11:33 PM
pelikan updated this revision to Diff 81923.Dec 19 2016, 12:40 AM
pelikan edited edge metadata.
pelikan marked 9 inline comments as done.

address comments

dberris requested changes to this revision.Dec 19 2016, 12:45 AM
dberris edited edge metadata.

Did the tests need updating, so that we know the "default" would do the right thing?

Currently we set the pattern explicitly. It'd be great to see a test that makes sure that the default pattern including argv[0] actually shows up.

This revision now requires changes to proceed.Dec 19 2016, 12:45 AM
pelikan updated this revision to Diff 81928.Dec 19 2016, 1:16 AM
pelikan edited edge metadata.

add a test

dberris requested changes to this revision.Dec 19 2016, 1:27 AM
dberris edited edge metadata.

I was thinking it might be better if you made a specific test just for this -- that the "default" (i.e. not setting the pattern with the XRAY_OPTIONS environment variable) would be better, so that we can isolate the feature in a specific test.

test/xray/TestCases/Linux/fixedsize-logging.cc
5 ↗(On Diff #81928)

I'm not sure we can rely on shell expansion to work here. Can you think of a different way of doing this, where you look for the filename on the "default" (i.e. non-configured explicitly) name here?

This revision now requires changes to proceed.Dec 19 2016, 1:27 AM
pelikan updated this revision to Diff 82567.Dec 27 2016, 5:41 PM
pelikan edited edge metadata.

sync; test changes still missing

pelikan updated this revision to Diff 82568.Dec 27 2016, 5:44 PM
pelikan edited edge metadata.

rebase this properly this time?

pelikan updated this revision to Diff 82581.Dec 27 2016, 10:04 PM
pelikan marked an inline comment as done.
  • make a bespoke test for argv[0] being in the file name
  • make it more complicated and not use $() and ??????
dberris accepted this revision.Jan 2 2017, 8:44 PM
dberris edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jan 2 2017, 8:44 PM
This revision was automatically updated to reflect the committed changes.