Set Default Behavior to Skip Re-Instrumentation of Duplicate Modules #86
Conversation
dillonfranke
commented
Dec 11, 2024
•
dillonfranke
commented
- Added a check that prevents subsequent modules of the same name from being re-instrumented. This is useful when instrumenting certain Apple frameworks which, as of MacOS Sequoia, now have the same name.
- Added a check to the Hook API to throw an error if the hook cannot be resolved. This provides feedback to the hook writer when the hook fails.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
tinyinst.cpp
Outdated
| @@ -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.
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.
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.
I suppose I could just change it to WARN()
README.md
Outdated
| @@ -188,6 +188,8 @@ In addition to the general-purpose API documented above, TinyInst also implement | |||
|
|
|||
| `-indirect_instrumentation [none|local|global|auto]` which instrumentation to use for indirect jump/calls | |||
|
|
|||
| `-ignore_duplicates_module [module name]` Ensures only the first loaded instance of `[module name]` is instrumented, ignoring subsequent duplicates. Useful when instrumenting system libraries (e.g., `CoreAudio`) on macOS Sequoia and later, where multiple distinct libraries with the same name may be loaded. | |||
There was a problem hiding this comment.
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.
Sounds good! I will go ahead and remove the flag and implement ignoring duplicate modules as the default behavior :)
Made skipping re-instrumentation of duplicate modules default behavior
removed duplicate module name addition
| @@ -850,6 +850,8 @@ void TinyInst::OnModuleInstrumented(ModuleInfo* module) { | |||
| } | |||
| if(address) { | |||
| resolved_hooks[address] = hook; | |||
| } else { | |||
| WARN("Could not resolve function %s in module %s", hook->GetFunctionName().c_str(), hook->GetModuleName().c_str()); | |||
There was a problem hiding this comment.
I think it only makes sense to issue this warning if hook->GetModuleName() != "*". Could you add this check?
dillonfranke
changed the title
added additional check for wildcard module before warning of failed hook resolution
|
Cool, I think we can merge now! |
