Skip to content

Conversation

@AnnaAr321
Copy link

The library work wonderfully (thanks!). In the commit, I added flags to opt out of tracking when I know it's not required.

an example of how to opt out from everything:

export INSTRUMENT_SLEEP=0
export INSTRUMENT_CONDVAR=0
export INSTRUMENT_MUTEX=0
export INSTRUMENT_SEMAPHORE=0
export INSTRUMENT_MISC=0

The library work wonderfully (thanks!). In the commit, I added flags to opt out of tracking when I know it's not required.

an example of how to opt out from everything:
```
export INSTRUMENT_SLEEP=0
export INSTRUMENT_CONDVAR=0
export INSTRUMENT_MUTEX=0
export INSTRUMENT_SEMAPHORE=0
export INSTRUMENT_MISC=0
```
Copy link
Owner

@dsharlet dsharlet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Sorry about the delay, I'm apparently not getting notified for things on this repo. I'll fix that.

The overall idea of this change makes sense, I have a few minor suggestions about the implementation.

extern "C" {

unsigned int sleep(unsigned int secs) {
if (!hooks::sleep) hooks::init(hooks::sleep, __sleep, "sleep");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little worried about the global constructor to initialize the flags. I'm wondering, would the following pattern work?

if (!hooks::sleep) g_instrument_sleep = hooks::init(hooks::sleep, __sleep, "sleep", "INSTRUMENT_SLEEP");
if (!g_instrument_sleep) {
  return hooks::sleep(secs);
}
  • Add a parameter to hooks::init that takes a string to indicate which environment variable should guard that function.
  • Return the value of the flag that controls instrumentation of that function.

Mostly it's the same as your change, just initializing the global flags differently to avoid potential global constructor ordering issues.

It assigns the same flag multiple times, which is a bit of a wart, but only a few times (once per function).

* It reads environment variables to configure which instrumentation points
* should be disabled. Instrumentation is enabled by default and is only
* disabled if the corresponding environment variable is explicitly set to "0".
* E.g. `export INSTRUMENT_SLEEP=0` will disable sleep instrumentation.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these environment variables be prefixed with PTHREAD_TRACE_, to avoid global name collisions? Maybe PTHREAD_TRACE_SLEEP, PTHREAD_TRACE_CONDVAR, etc.?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants