Mark crashpad_handler testonly on Android and remove crashpad_handler_named_as_so [crashpad/crashpad : main]
Andrew Grieve (Gerrit)
Andrew Grieve added 5 comments![]( "Open in Gerrit")
//! \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.Andrew GrieveI 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.
Fair enough. I'll find a spot in chromium for the helper. Ideally crashpad would have an android test that uses an .apk, and so would use this, but it uses a raw executable to run tests, and so cannot test the logic that is actually used by chrome, where the hander exists within the .apk, and is launched with `app_process`
#include "base/files/file_path.h"Andrew GrieveCrashpad allows the #include that’s already in the lead header (`client/crashpad_client.h`) to fulfill the IWYU need for this.
Acknowledged
}
if (!crashpad_is_ios) {Andrew GrieveThe 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?
Good catch! Was the result of some flip flopping on my part.
# 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 = trueAndrew Grieve(see below.)
Acknowledged
# 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.Andrew GrieveWhy 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.
GN's autoformatter recently learned to reflow comments. This sort of change has been happening all over chromium as of late...
Related details
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Grieve (Gerrit)
Andrew Grieve added 6 comments![]( "Open in Gerrit")
* Adds GetHandlerLinkerPathsAndEnv(), since apps should not be
responsible for coming up with this logic on their own.Andrew GrieveSeeing how this works out, I am less convinced about this part.
Done
Bug: chromium:425948259Andrew Grievechromium: prefix not necessary in the merged bug namespace.
Done
LOG(ERROR) << "dladdr failed";Andrew GrieveUse `dlerror` when any of the `<dlfcn.h>` functions fail.
Acknowledged
base::FilePath handler_trampoline_path =
libdir.Append("libcrashpad_handler_trampoline.so");Andrew GrieveThis is also probably assuming too much.
Acknowledged
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);
}Andrew GrieveDoesn’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.
AFAICT, this is the standard way...
handler_trampoline->swap(local_handler_trampoline);
handler_library->swap(local_handler_library);
env->swap(local_env);Andrew GrieveThis 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?
Removed.
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 1 comment![]( "Open in Gerrit")
# 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.Andrew GrieveWhy 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.
GN's autoformatter recently learned to reflow comments. This sort of change has been happening all over chromium as of late...
GN's autoformatter recently learned to reflow comments. This sort of change has been happening all over chromium as of late...
“The tool suddenly does the wrong thing, but it does it automatically” isn’t an excuse. Why can’t the tool be made to do the right thing?
Reference to the GN change?
Is there a bug filed to get it to use the full 80 characters like the rest of Chromium?
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. |
Andrew Grieve (Gerrit)
Andrew Grieve added 1 comment![]( "Open in Gerrit")
# 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.Andrew GrieveWhy 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.
Mark MentovaiGN's autoformatter recently learned to reflow comments. This sort of change has been happening all over chromium as of late...
GN's autoformatter recently learned to reflow comments. This sort of change has been happening all over chromium as of late...
“The tool suddenly does the wrong thing, but it does it automatically” isn’t an excuse. Why can’t the tool be made to do the right thing?
Reference to the GN change?
Is there a bug filed to get it to use the full 80 characters like the rest of Chromium?
https://gn-review.googlesource.com/c/gn/+/21820
Looks like the "t" of "target" was at column 82, so likely that's what it was wrapped.
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 1 comment![]( "Open in Gerrit")
# 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.Andrew GrieveWhy 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.
Mark MentovaiGN's autoformatter recently learned to reflow comments. This sort of change has been happening all over chromium as of late...
Andrew GrieveGN's autoformatter recently learned to reflow comments. This sort of change has been happening all over chromium as of late...
“The tool suddenly does the wrong thing, but it does it automatically” isn’t an excuse. Why can’t the tool be made to do the right thing?
Reference to the GN change?
Is there a bug filed to get it to use the full 80 characters like the rest of Chromium?
https://gn-review.googlesource.com/c/gn/+/21820
Looks like the "t" of "target" was at column 82, so likely that's what it was wrapped.
```
12345678901234567890123456789012345678901234567890123456789012345678901234567890
# 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
# target directly. Instead, use crashpad_handler_console.
```
Looks like 80 to me.
Betcha something’s counting a byte length and not a string length. This is UTF-8. ’ is 1 character but 3 bytes. The line is 80 characters but 83 bytes.
Related details
- Andrew Grieve
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Grieve (Gerrit)
Andrew Grieve added 1 comment![]( "Open in Gerrit")
# 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.Andrew GrieveWhy 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.
Mark MentovaiGN's autoformatter recently learned to reflow comments. This sort of change has been happening all over chromium as of late...
Andrew GrieveGN's autoformatter recently learned to reflow comments. This sort of change has been happening all over chromium as of late...
“The tool suddenly does the wrong thing, but it does it automatically” isn’t an excuse. Why can’t the tool be made to do the right thing?
Reference to the GN change?
Is there a bug filed to get it to use the full 80 characters like the rest of Chromium?
Mark Mentovaihttps://gn-review.googlesource.com/c/gn/+/21820
Looks like the "t" of "target" was at column 82, so likely that's what it was wrapped.
```
12345678901234567890123456789012345678901234567890123456789012345678901234567890
# 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
# target directly. Instead, use crashpad_handler_console.
```Looks like 80 to me.
Betcha something’s counting a byte length and not a string length. This is UTF-8. ’ is 1 character but 3 bytes. The line is 80 characters but 83 bytes.
Oh, you're right! vim showed it as 83, but python3 agrees it's only 80. Your explanation must bet it!
Anyways, I've reverted it, but unless the GN bug is fixed, it'll just get re-wrapped the next time someone touches the file.
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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
2 comments
Oh, you're right! vim showed it as 83, but python3 agrees it's only 80. Your explanation must bet it!
Anyways, I've reverted it, but unless the GN bug is fixed, it'll just get re-wrapped the next time someone touches the file.
Yeah, I just talked to Will, he’s OK with fixing GN to interpret UTF-8 characters and not bytes. Something else for the TODO list.
Related details
- Andrew Grieve
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Grieve (Gerrit)
Andrew Grieve voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Grieve (Gerrit)
Andrew Grieve voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
crashpad-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
crashpa...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Mark crashpad_handler testonly on Android and remove crashpad_handler_named_as_so
The only supported way to use Crashpad on Android is to via:
* StartHandlerWithLinker*
* StartJavaHandler*
The stand-alone handler binary is not longer used, except in crashpad's
own tests.
- M BUILD.gn
- M client/BUILD.gn
- M handler/BUILD.gn
Code-Review: +1 by Mark Mentovai
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |