Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to change signals #22

Open
Bojun-Seo opened this issue Mar 7, 2023 · 8 comments
Open

Add option to change signals #22

Bojun-Seo opened this issue Mar 7, 2023 · 8 comments

Comments

@Bojun-Seo
Copy link
Collaborator

Bojun-Seo commented Mar 7, 2023

heaptrace use SIGUSR1 and SIGUSR2 to dump allocation status (in terms of size and count respectively) during process running.
But the signal handler will not work properly, if the target process uses one or both of the signals.

So I suggest to add option to change signals to dump allocation status.
For example, (only to show the intention not an actual option format)

# register signal 33 to dump the allocation status sorted in terms of size
./heaptrace --signal size:33 ./a.out

# register signal 34 to dump the allocation status sorted in terms of count
./heaptrace --signal count:34 ./a.out

# register signal 33 and 34 to dump the allocation status sorted in terms of size and count
./heaptrace --signal size:33 --signal count:34 ./a.out
@honggyukim
Copy link
Owner

Yes, that's doable, but we can also support signal name as well as signal number.

For example, the following commands do the same thing.

$ heaptrace --signal size:27 ./a.out

$ heaptrace --signal size:SIGPROF ./a.out

The signal number of SIGPROF is 27. Would you like to implement it?

@Bojun-Seo
Copy link
Collaborator Author

Yes, I'd like to implement it

@Bojun-Seo
Copy link
Collaborator Author

Bojun-Seo commented Nov 15, 2023

I suggested to register multiple keys like followings:

./heaptrace --signal size:33 --signal count:34 ./a.out

But how about registering multiple keys like this?(comma separate)

./heaptrace --signal size:33,count:34 ./a.out

Whatever the signal option interface looks like, I'm thinking about to set HEAPTRACE_SIGNAL environmental variable like following.
HEAPTRACE_SIGNAL=size:33,count:34
The second form make it easy to fabricate this environmental variable.
And some users may want to use libheaptrace.so directly after setting environment variables. The second form would be more intuitive for this kind of users as well.

Or

We could think about another form to set environmental variable.

Or

We could think about to allow only one signal and sort key pair can be set.

@Bojun-Seo
Copy link
Collaborator Author

I almost implement this feature except to convert SIGXX into corresponding signal number. It seems very hard to do this neatly. So I suggest to support only signal numbers, not signal macro names.

@honggyukim
Copy link
Owner

You can simply use the std::map as follows.

std::map<std::string, int> signum = {
        {"SIGHUP", 1},
        {"SIGINT", 2},
        {"SIGQUIT", 3},
        {"SIGILL", 4},
        {"SIGTRAP", 5},
        {"SIGABRT", 6},
        {"SIGBUS", 7},
        {"SIGFPE", 8},
        {"SIGKILL", 9},
        {"SIGUSR1", 10},
        {"SIGSEGV", 11},
        {"SIGUSR2", 12},
        {"SIGPIPE", 13},
        {"SIGALRM", 14},
        {"SIGTERM", 15},
        {"SIGSTKFLT", 16},
        {"SIGCHLD", 17},
        {"SIGCONT", 18},
        {"SIGSTOP", 19},
        {"SIGTSTP", 20},
        {"SIGTTIN", 21},
        {"SIGTTOU", 22},
        {"SIGURG", 23},
        {"SIGXCPU", 24},
        {"SIGXFSZ", 25},
        {"SIGVTALRM", 26},
        {"SIGPROF", 27},
        {"SIGWINCH", 28},
        {"SIGIO", 29},
        {"SIGPWR", 30},
        {"SIGSYS", 31},
};

@honggyukim
Copy link
Owner

honggyukim commented Nov 18, 2023

/usr/include/asm-generic/signal.h says the SIGRTMIN and above should not be considered constants, which means the signal number might be different depending on the systems so we can't support it.
https://github.com/torvalds/linux/blob/v6.6/include/uapi/asm-generic/signal.h#L49-L50

@Bojun-Seo
Copy link
Collaborator Author

You are right, so I updated PR without supporting signal definition string.

@Bojun-Seo
Copy link
Collaborator Author

I discussed about the option format with my coworkers and followings are the suggested candidates.

heaptrace --signal size:33,count:34 ./a.out
heaptrace --signal size:33 --signal count:34 ./a.out
heaptrace --sig-size 33 --sig-count 34 ./a.out
heaptrace --signal 33,34 ./a.out // set sort key implicitly, size for 33 and count for 34
heaptrace --signal ,34 ./a.out // In case to set count for 34 only
heaptrace --signal 33, ./a.out // In case to set size for 33 only
heaptrace --SIG 33,34 ./a.out // upper or lower
heaptrace --sig-for-size 33 --sig-for-count 34 ./a.out
heaptrace --sig4size 33 --sig4count 34 ./a.out
heaptrace --signal s:33,c:34 ./a.out

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

No branches or pull requests

2 participants