Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2156173002:
Don't define |r_debug| and |link_map| on Android releases 21 and later
|
Created:
4 years, 5 months ago by tzimmermann Modified:
4 years, 4 months ago Reviewers:
rmcilroy CC:
google-breakpad-dev_googlegroups.com, Ted Mielczarek Base URL:
https://chromium.googlesource.com/breakpad/breakpad.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionDon't define |r_debug| and |link_map| on Android releases 21 and later
NDKs for Android 21 and later have the data structures |r_debug| and
|link_map| defined in their header files. Defining them multiple times
generates a compiler error.
This patch protects both data structures from definition on Android 21
and later.
BUG=629088
R=rmcilroy@chromium.org
Committed: https://chromium.googlesource.com/breakpad/breakpad/+/0ebdc4a10a506e2a4a3a039c479b40219a84b760
Committed: https://chromium.googlesource.com/breakpad/breakpad/+/0fc6d0c8dfbb6e4226fd79c622b701a62c901f14
Patch Set 1 #
Total comments: 1
Patch Set 2 : Don't redefine |r_debug| and |link_map| on Android releases 21 and later #
Total comments: 1
Patch Set 3 : Don't redefine |r_debug| and |link_map| on Android releases 21 and later #Patch Set 4 : Don't redefine |r_debug| and |link_map| on Android releases 21 and later #
Total comments: 1
Patch Set 5 : Don't redefine |r_debug| and |link_map| on Android releases 21 and later #Patch Set 6 : Don't redefine |r_debug| and |link_map| on Android releases 21 and later #
Total comments: 1
Patch Set 7 : Don't redefine |r_debug| and |link_map| on Android releases 21 and later #Patch Set 8 : Don't redefine |r_debug| and |link_map| on Android releases 21 and later #Patch Set 9 : Don't redefine |r_debug| and |link_map| on Android releases 21 and later #Messages
Total messages: 30 (8 generated)
Description was changed from ========== Don't define |r_debug| and |link_map| on Android releases 21 and later NDKs for Android 21 and later have the data structures |r_debug| and |link_map| defined in their header files. Defining them multiple times generates a compiler error. This patch protects both data structures from definition on Android 21 and later. BUG=629088 ========== to ========== Don't define |r_debug| and |link_map| on Android releases 21 and later NDKs for Android 21 and later have the data structures |r_debug| and |link_map| defined in their header files. Defining them multiple times generates a compiler error. This patch protects both data structures from definition on Android 21 and later. BUG=629088 ==========
tzimmermann@mozilla.com changed reviewers: + rmcilroy@chromium.org
https://codereview.chromium.org/2156173002/diff/1/src/common/android/include/... File src/common/android/include/link.h (right): https://codereview.chromium.org/2156173002/diff/1/src/common/android/include/... src/common/android/include/link.h:47: #if defined(ANDROID) && ANDROID_VERSION <= 20 I don't think the defined(ANDROID) is needed - these headers are only included if building for Android. Also, where does ANDROID_VERSION come from? I don't think it is defined in the NDK and we don't define it either in Breakpad or Chromium's build as far as I can tell. Instead you might have to do something like what's done in elf.h where we do, for example: #define Elf32_auxv_t __bionic_Elf32_auxv_t #define Elf64_auxv_t __bionic_Elf64_auxv_t Then undef this definition and redefine the structs here.
Hi! On 2016/07/18 15:04:08, rmcilroy wrote: > Also, where does ANDROID_VERSION come from? I don't think it is defined in the > NDK and we don't define it either in Breakpad or Chromium's build as far as I > can tell. That's a side effect from how we build this code in Gecko. I didn't expect other code to be affected as well. > Instead you might have to do something like what's done in elf.h where we do, > for example: > #define Elf32_auxv_t __bionic_Elf32_auxv_t > #define Elf64_auxv_t __bionic_Elf64_auxv_t > > Then undef this definition and redefine the structs here. I'll fix the patch set accordingly.
This is my first patch. Please run the try jobs once you're OK with them. Thanks!
On 2016/07/19 06:46:19, tzimmermann wrote: > This is my first patch. Please run the try jobs once you're OK with them. > Thanks! s/them/it
LGTM, thanks. There are no bots for breakpad unfortunately. I'll land this for you, but could you also create a roll CL for Chromium once this lands (see https://codereview.chromium.org/1655463002/ for an example) and then that can be run on the Chromium waterfall to ensure nothing breaks.
Description was changed from ========== Don't define |r_debug| and |link_map| on Android releases 21 and later NDKs for Android 21 and later have the data structures |r_debug| and |link_map| defined in their header files. Defining them multiple times generates a compiler error. This patch protects both data structures from definition on Android 21 and later. BUG=629088 ========== to ========== Don't define |r_debug| and |link_map| on Android releases 21 and later NDKs for Android 21 and later have the data structures |r_debug| and |link_map| defined in their header files. Defining them multiple times generates a compiler error. This patch protects both data structures from definition on Android 21 and later. BUG=629088 R=rmcilroy@chromium.org Committed: https://chromium.googlesource.com/breakpad/breakpad/+/0ebdc4a10a506e2a4a3a039... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 0ebdc4a10a506e2a4a3a039c479b40219a84b760 (presubmit successful).
Message was sent while issue was closed.
hush@chromium.org changed reviewers: + hush@chromium.org
Message was sent while issue was closed.
this does not compile:
../../breakpad/src/client/linux/minidump_writer/minidump_writer.cc: In member
function 'bool
{anonymous}::MinidumpWriter::WriteDSODebugStream(MDRawDirectory*)':
../../breakpad/src/client/linux/minidump_writer/minidump_writer.cc:726:20:
error: aggregate
'{anonymous}::MinidumpWriter::WriteDSODebugStream(MDRawDirectory*)::r_debug
debug_entry' has incomplete type and cannot be defined
struct r_debug debug_entry;
^
../../breakpad/src/client/linux/minidump_writer/minidump_writer.cc:732:23:
error: aggregate
'{anonymous}::MinidumpWriter::WriteDSODebugStream(MDRawDirectory*)::link_map
map' has incomplete type and cannot be defined
struct link_map map;
^
../../breakpad/src/client/linux/minidump_writer/minidump_writer.cc:752:25:
error: aggregate
'{anonymous}::MinidumpWriter::WriteDSODebugStream(MDRawDirectory*)::link_map
map' has incomplete type and cannot be defined
struct link_map map;
Message was sent while issue was closed.
#undef r_debug #undef link_map these 2 lines seem to be the culprit. Can you fix?
Message was sent while issue was closed.
https://codereview.chromium.org/2156173002/diff/20001/src/common/android/incl... File src/common/android/include/link.h (right): https://codereview.chromium.org/2156173002/diff/20001/src/common/android/incl... src/common/android/include/link.h:44: the block after line 48 won't be applied to 64 bit devices, which means r_debug and link_map won't be defined
Message was sent while issue was closed.
Sorry for that mess. Patch set 3 separates the undef from all platforms that don't have the struct's defined. I've successfully build this patch on arm, x86 and x86-64.
Message was sent while issue was closed.
On 2016/07/20 09:07:45, tzimmermann wrote: > Sorry for that mess. > > Patch set 3 separates the undef from all platforms that don't have the struct's > defined. I've successfully build this patch on arm, x86 and x86-64. Sorry about that hush@, I've reverted this CL in https://codereview.chromium.org/2163923002/. Thomas, could you re-upload the patch to a new CL with the updates please.
Message was sent while issue was closed.
On 2016/07/20 10:28:54, rmcilroy wrote: > Sorry about that hush@, I've reverted this CL in > https://codereview.chromium.org/2163923002/. > > Thomas, could you re-upload the patch to a new CL with the updates please. Patch set 4 is patch set 3 + a rebase onto rev b5200a9 from master.
Message was sent while issue was closed.
Description was changed from ========== Don't define |r_debug| and |link_map| on Android releases 21 and later NDKs for Android 21 and later have the data structures |r_debug| and |link_map| defined in their header files. Defining them multiple times generates a compiler error. This patch protects both data structures from definition on Android 21 and later. BUG=629088 R=rmcilroy@chromium.org Committed: https://chromium.googlesource.com/breakpad/breakpad/+/0ebdc4a10a506e2a4a3a039... ========== to ========== Don't define |r_debug| and |link_map| on Android releases 21 and later NDKs for Android 21 and later have the data structures |r_debug| and |link_map| defined in their header files. Defining them multiple times generates a compiler error. This patch protects both data structures from definition on Android 21 and later. BUG=629088 R=rmcilroy@chromium.org Committed: https://chromium.googlesource.com/breakpad/breakpad/+/0ebdc4a10a506e2a4a3a039... ==========
rmcilroy@chromium.org changed reviewers: - hush@chromium.org
https://codereview.chromium.org/2156173002/diff/60001/src/common/android/incl... File src/common/android/include/link.h (right): https://codereview.chromium.org/2156173002/diff/60001/src/common/android/incl... src/common/android/include/link.h:79: #include_next <link.h> Instead of including this twice (and doing the one above inside a "extern "C" {" which might change semantics), could you instead do the #ifdef check around the #define / #undef code.
Patch set 6 duplicates the ifdef conditional, so that there's only one include statement.
LGTM with a comment. https://codereview.chromium.org/2156173002/diff/100001/src/common/android/inc... File src/common/android/include/link.h (right): https://codereview.chromium.org/2156173002/diff/100001/src/common/android/inc... src/common/android/include/link.h:46: #endif // !defined(__aarch64__) && !defined(__x86_64__) Please update comment with Mips (and below)
I updated the comments.
LGTM, thanks.
Description was changed from ========== Don't define |r_debug| and |link_map| on Android releases 21 and later NDKs for Android 21 and later have the data structures |r_debug| and |link_map| defined in their header files. Defining them multiple times generates a compiler error. This patch protects both data structures from definition on Android 21 and later. BUG=629088 R=rmcilroy@chromium.org Committed: https://chromium.googlesource.com/breakpad/breakpad/+/0ebdc4a10a506e2a4a3a039... ========== to ========== Don't define |r_debug| and |link_map| on Android releases 21 and later NDKs for Android 21 and later have the data structures |r_debug| and |link_map| defined in their header files. Defining them multiple times generates a compiler error. This patch protects both data structures from definition on Android 21 and later. BUG=629088 R=rmcilroy@chromium.org Committed: https://chromium.googlesource.com/breakpad/breakpad/+/0ebdc4a10a506e2a4a3a039... Committed: https://chromium.googlesource.com/breakpad/breakpad/+/0fc6d0c8dfbb6e4226fd79c... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) manually as 0fc6d0c8dfbb6e4226fd79c622b701a62c901f14 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Don't define |r_debug| and |link_map| on Android releases 21 and later NDKs for Android 21 and later have the data structures |r_debug| and |link_map| defined in their header files. Defining them multiple times generates a compiler error. This patch protects both data structures from definition on Android 21 and later. BUG=629088 R=rmcilroy@chromium.org Committed: https://chromium.googlesource.com/breakpad/breakpad/+/0ebdc4a10a506e2a4a3a039... Committed: https://chromium.googlesource.com/breakpad/breakpad/+/0fc6d0c8dfbb6e4226fd79c... ========== to ========== Don't define |r_debug| and |link_map| on Android releases 21 and later NDKs for Android 21 and later have the data structures |r_debug| and |link_map| defined in their header files. Defining them multiple times generates a compiler error. This patch protects both data structures from definition on Android 21 and later. BUG=629088 R=rmcilroy@chromium.org Committed: https://chromium.googlesource.com/breakpad/breakpad/+/0ebdc4a10a506e2a4a3a039... Committed: https://chromium.googlesource.com/breakpad/breakpad/+/0fc6d0c8dfbb6e4226fd79c... ==========
I've reopened this issue because the patch was reverted. The problem is a multi-line comment. See https://bugs.chromium.org/p/chromium/issues/detail?id=629088#c3
Sorry that this trivial change is taking so much effort. I've replaced the multi-line comment on line 84 with two separate comments.
And fixed another multi-line comment near line 46.
Ping for another review. I try to test as good as I can (Linux, Android). Is there anything else I can do to avoid backouts? |
