-
Notifications
You must be signed in to change notification settings - Fork 119
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
Set Default Behavior to Skip Re-Instrumentation of Duplicate Modules #86
Changes from 3 commits
2a54040
8517901
88b42ad
39bad59
610d10a
4ffa8c0
9facb93
61c47b1
521ec50
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,7 @@ ModuleInfo::ModuleInfo() { | |
max_address = 0; | ||
loaded = false; | ||
instrumented = false; | ||
ignore_duplicates = false; | ||
instrumented_code_local = NULL; | ||
instrumented_code_remote = NULL; | ||
instrumented_code_remote_previous = NULL; | ||
|
@@ -850,6 +851,8 @@ void TinyInst::OnModuleInstrumented(ModuleInfo* module) { | |
} | ||
if(address) { | ||
resolved_hooks[address] = hook; | ||
} else { | ||
FATAL("Could not resolve function %s in module %s", hook->GetFunctionName().c_str(), hook->GetModuleName().c_str()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't want to FATAL() here because hook's module name could be "*" so it searches for the function in all modules. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah that makes good sense. Do you foresee a better way to alert the user if a hook function is unable to be resolved? Or do you think it's not necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose I could just change it to WARN() |
||
} | ||
} | ||
} | ||
|
@@ -1075,9 +1078,14 @@ void TinyInst::OnInstrumentModuleLoaded(void *module, ModuleInfo *target_module) | |
target_module->module_header && | ||
(target_module->module_header != (void *)module)) | ||
{ | ||
WARN("Instrumented module loaded on a different address than seen previously\n" | ||
"Module will need to be re-instrumented. Expect a drop in performance."); | ||
ClearInstrumentation(target_module); | ||
if (target_module->ignore_duplicates) { | ||
WARN("Skipping duplicate module %s.", target_module->module_name.c_str()); | ||
return; | ||
} else { | ||
WARN("Instrumented module loaded on a different address than seen previously\n" | ||
"Module will need to be re-instrumented. Expect a drop in performance."); | ||
ClearInstrumentation(target_module); | ||
} | ||
} | ||
|
||
target_module->module_header = (void *)module; | ||
|
@@ -1095,7 +1103,7 @@ void TinyInst::OnInstrumentModuleLoaded(void *module, ModuleInfo *target_module) | |
} | ||
} | ||
|
||
// called when a potentialy interesting module gets loaded | ||
// called when a potentially interesting module gets loaded | ||
void TinyInst::OnModuleLoaded(void *module, char *module_name) { | ||
Debugger::OnModuleLoaded(module, module_name); | ||
|
||
|
@@ -1277,6 +1285,9 @@ void TinyInst::Init(int argc, char **argv) { | |
std::list <char *> module_names; | ||
GetOptionAll("-instrument_module", argc, argv, &module_names); | ||
|
||
std::list <char *> ignored_duplicate_modules; | ||
GetOptionAll("-ignore_duplicates_module", argc, argv, &ignored_duplicate_modules); | ||
|
||
#if defined(__APPLE__) && defined(ARM64) | ||
std::set <std::string> orig_uniq_mod_names; | ||
std::set <std::string> new_uniq_mod_names; | ||
|
@@ -1314,6 +1325,13 @@ void TinyInst::Init(int argc, char **argv) { | |
#endif | ||
|
||
for (const auto module_name: module_names) { | ||
ModuleInfo *new_module = new ModuleInfo(); | ||
new_module->module_name = module_name; | ||
for (const auto& ignored_module : ignored_duplicate_modules) { | ||
if (strcmp(ignored_module, module_name) == 0) { | ||
new_module->ignore_duplicates = true; | ||
} | ||
} | ||
AddInstrumentedModule(module_name, true); | ||
// SAY("--- %s\n", module_name); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having a flag for this is not needed; TinyInst will not currently work correctly with duplicate module names, so the current ignore_duplicates behavior should just be the default implementation for all modules. Just issuing a warning on duplicate module and not instrumenting it is completely sufficient.
Later we need to think how to resolve this e.g. if the user wants to instrument the second module with the same name (or multiple modules with the same name). For that, there should probably be an option to specify instrumented_module by full path, but we can figure that out later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! I will go ahead and remove the flag and implement ignoring duplicate modules as the default behavior :)