Add option to change signals - #33
Open
Bojun-Seo wants to merge 2 commits into
Open
Conversation
Add signal handler registration interface and modularize signal handler registration Signed-off-by: Bojun Seo <bojun.seo.0@gmail.com>
User could register signal to report on specific sort key. The option format is "KEY1:SIGNO1,KEY2:SIGNO2". The usage is as follows: $ heaptrace --signal size:35 samples/samples.out $ heaptrace --signal count:35 samples/samples.out $ heaptrace --signal count:35,size:36 samples/samples.out Signed-off-by: Bojun Seo <bojun.seo.0@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
Adds support for customizing which signals trigger heap allocation dumps, to avoid conflicts with user processes.
- Refactors signal registration into
register_sighandlerand exposes handlers for size/count - Parses a new
--signalsCLI option (andHEAPTRACE_SIGNALSenv) to map keys (size/count) to arbitrary signal numbers - Updates headers, initialization logic, and child-environment setup to propagate the new option
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sighandler.h | Declares register_sighandler and new handler functions |
| src/sighandler.cc | Refactors signal setup; defines size_sighandler/count_sighandler |
| src/libheaptrace.cc | Reads HEAPTRACE_SIGNALS, parses key:number pairs, registers handlers |
| src/heaptrace.h | Adds signals field to opts |
| src/heaptrace.cc | Introduces --signals CLI option and exports via env |
Comments suppressed due to low confidence (2)
src/heaptrace.cc:68
- There are no tests covering parsing or handling of the new
--signalsoption; consider adding unit or integration tests to verify correct behavior and error handling.
opts->signals = arg;
src/sighandler.cc:44
- [nitpick] The
size_sighandlerandsigusr1_handlerfunctions both calldump_stackmap("size", ...); consider consolidating to a single handler to avoid duplicate logic.
void size_sighandler(int /*unused*/)
| for (const auto& key_num: key_num_vec) { | ||
| auto item_vec = utils::string_split(key_num, ':'); | ||
| if (item_vec.size() != 2) { | ||
| pr_out("Failed to parsing signals: %s\n", |
There was a problem hiding this comment.
The message "Failed to parsing signals" is grammatically incorrect; consider "Failed to parse signal definition: %s".
Suggested change
| pr_out("Failed to parsing signals: %s\n", | |
| pr_out("Failed to parse signals: %s\n", |
| } else if (key == "count") { | ||
| register_sighandler(count_sighandler, signo); | ||
| } else { | ||
| pr_out("Cannot register not supporting key: %s\n", |
There was a problem hiding this comment.
The error text is awkward; consider "Unsupported key: %s" for clarity.
Suggested change
| pr_out("Cannot register not supporting key: %s\n", | |
| pr_out("Unsupported key: %s\n", |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implementation of issue #22
heaptrace uses
SIGUSR1andSIGUSR2to 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,