Remove legacy Android APIs, mark crashpad_handler testonly on Android [crashpad/crashpad : main]
Andrew Grieve (Gerrit)
Andrew Grieve voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrew Grieve
| Commit-Queue | +1 |
1 comment
Gerrit UI is showing "Merge Conflict", but the parent commit is already at tip-of-tree, so I'm not sure why it's showing that...
Related details
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 11 comments![]( "Open in Gerrit")
* Adds GetHandlerLinkerPathsAndEnv(), since apps should not be
responsible for coming up with this logic on their own.Seeing how this works out, I am less convinced about this part.
Bug: chromium:425948259chromium: prefix not necessary in the merged bug namespace.
//! \param[in] symbol An address of a symbol in the calling module (e.g. a
//! function in the test or chrome library). This is used by `dladdr` to
//! identify the library.I don’t really love this approach. It seems kind of specific to Chrome’s precise use. I’m a little inclined to just make the caller work it out.
#include "base/files/file_path.h"Crashpad allows the #include that’s already in the lead header (`client/crashpad_client.h`) to fulfill the IWYU need for this.
LOG(ERROR) << "dladdr failed";Use `dlerror` when any of the `<dlfcn.h>` functions fail.
base::FilePath handler_trampoline_path =
libdir.Append("libcrashpad_handler_trampoline.so");This is also probably assuming too much.
for (char** envp = environ; *envp != nullptr; ++envp) {
std::string env_var(*envp);
if (env_var.compare(0, 16, "LD_LIBRARY_PATH=") == 0) {
env_var = "LD_LIBRARY_PATH=" + libdir_str + ":" + env_var.substr(16);
found_ld_library_path = true;
}
local_env.push_back(env_var);
}
if (!found_ld_library_path) {
local_env.push_back("LD_LIBRARY_PATH=" + libdir_str);
}Doesn’t Bionic have its own environment-manipulation functions per standard C?
If you’ve got a reason to not use that, you’e got to say so in a comment.
handler_trampoline->swap(local_handler_trampoline);
handler_library->swap(local_handler_library);
env->swap(local_env);This function can’t fail (after `dladdr`, which happens early enough that none of these are a concern), so what’s the point of the local/swap stuff?
}
if (!crashpad_is_ios) {The block above that’s ending on line 169 is already `if (!crashpad_is_ios)`. Why break it up into a second conditional that tests the same thing?
# crashpad_handler_trampoline is used on Android, but crashpad_handler is
# needed by crashpad_client_linux_test.cc (which cannot use the
# trampoline because it runs as a normal executable, which it does
# because it does not have access to chromium's base::MultiProcessTest).
testonly = true(see below.)
# Avoid .exp, .ilk, and .lib file collisions with crashpad_handler.exe by
# having this target produce crashpad_handler_com.com. Don’t use this
# target directly. Instead, use crashpad_handler_console.Why was this reformatted? The only change I see is that it’s being rewrapped, but the existing comment already fit within 80 columns, so the new wrapping is less correct.
In fact, now that I look at it (which I wouldn’t have done until this came along), the block at line 207 seems to have wrapped at 79 instead of 80 also.
Related details
- Andrew Grieve
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |