Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1637473003:
android: Use libc++ instead of stlport. (Closed)
|
Created:
4 years, 11 months ago by Michael Achenbach Modified:
4 years, 10 months ago CC:
JF, v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Descriptionandroid: Use libc++ instead of stlport.
Useful for example for using atomicops_internal_portable.h on Android.
BUG=v8:4615
LOG=y
patch from issue 1525813002 at patchset 1 (http://crrev.com/1525813002#ps1)
Committed: https://crrev.com/ee57e14c6f43e40d097456f23e6af8c27ef35e34
Cr-Commit-Position: refs/heads/master@{#33804}
Patch Set 1 #Patch Set 2 : Path fixes. #Patch Set 3 : Try using -std=c++11 #
Total comments: 3
Patch Set 4 : Review #Patch Set 5 : Nico's patch 2 from https://codereview.chromium.org/1641303004 #Patch Set 6 : Port https://codereview.chromium.org/1647493002/ #
Total comments: 2
Patch Set 7 : Use -std=c++11 #Patch Set 8 : Review jochen #
Total comments: 4
Patch Set 9 : Remove unnecessary ldflags. #Messages
Total messages: 43 (15 generated)
Description was changed from ========== android: Use libc++ instead of stlport. Useful for example for using atomicops_internal_portable.h on Android. BUG=none LOG=y patch from issue 1525813002 at patchset 1 (http://crrev.com/1525813002#ps1) ========== to ========== android: Use libc++ instead of stlport. Useful for example for using atomicops_internal_portable.h on Android. BUG=v8:4615 LOG=y patch from issue 1525813002 at patchset 1 (http://crrev.com/1525813002#ps1) ==========
Patchset #2 (id:20001) has been deleted
machenbach@chromium.org changed reviewers: + jochen@chromium.org, thakis@chromium.org
PTAL. @Nico: I hijacked your CL https://codereview.chromium.org/1525813002 and fixed some paths in gyp (could probably reduce some duplication though). Passes runhooks now. It spits out a gazillion of errors. Does it not compile with c++11? Do we also need to port more from https://codereview.chromium.org/951983002 ?
perhaps we should start passing -std=gnu++11 instead of -std=gnu++0x
On 2016/01/26 11:54:00, jochen wrote: > perhaps we should start passing -std=gnu++11 instead of -std=gnu++0x Hmm patch 3 didn't help. Checked locally that -std=gnu++11 is actually passed, and it is.
https://codereview.chromium.org/1637473003/diff/60001/build/standalone.gypi File build/standalone.gypi (right): https://codereview.chromium.org/1637473003/diff/60001/build/standalone.gypi#n... build/standalone.gypi:1012: '-std=c++11' ], Do you need gnu++11 instead of c++11 maybe? In Chromium, I think we still use -std=gnu++11 for android and linux builds (for linux builds it's only needed for one little thing, but I don't know about android). I don't think this causes the bot problems though, and if you can get away with c++11, it's better than gnu++11. It's different from the lhs though -- you get some fewer GNU extensions this way. https://codereview.chromium.org/1637473003/diff/60001/build/standalone.gypi#n... build/standalone.gypi:1027: '-I<(android_include)', I think the problem is this line. We don't add android_ndk_include (the corresponding thing in chromium) as include path. Read third_party/android-tools/ndk/sources/android/support/include/wchar.h: The support headers are supposed to shadow the android ndk libc headers, but since you add android_include before isystem<(android_support_include), the libc headers get picked up first. You at least need to move this line behind the support_include line, but you can probably get rid of it completely.
https://codereview.chromium.org/1637473003/diff/60001/build/standalone.gypi File build/standalone.gypi (right): https://codereview.chromium.org/1637473003/diff/60001/build/standalone.gypi#n... build/standalone.gypi:1027: '-I<(android_include)', On 2016/01/26 15:02:57, Nico wrote: > I think the problem is this line. We don't add android_ndk_include (the > corresponding thing in chromium) as include path. > > Read third_party/android-tools/ndk/sources/android/support/include/wchar.h: The > support headers are supposed to shadow the android ndk libc headers, but since > you add android_include before isystem<(android_support_include), the libc > headers get picked up first. You at least need to move this line behind the > support_include line, but you can probably get rid of it completely. Tried all combinations locally. The one you see here + line removed + -std=c++11. Didn't work.
Thanks! (I have a linux box -- but even there, `git cl upload` only works with `--bypass-hooks` for me.) It works if you use -isystem for android_include -- user include dirs are searched before system include paths. This is patch set 3 at https://codereview.chromium.org/1641303004 . I'd however recommend patch set 2 there, which uses --sysroot to point to the sysroot instead. (That's also what the chromium build does.) (I only tried building a couple hundred translation units, but that worked fine.) In addition to that change, I'd also recommend doing something like https://codereview.chromium.org/1647493002/ -- libc++ only needs to be in the include flags for .cc files, not for .c files.
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1637473003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1637473003/120001
PTAL. Patch 5 and 6 incorporate Nico's patch 2 from https://codereview.chromium.org/1641303004 and also https://codereview.chromium.org/1647493002/ . Thanks for your help Nico!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1637473003/diff/120001/build/standalone.gypi File build/standalone.gypi (right): https://codereview.chromium.org/1637473003/diff/120001/build/standalone.gypi#... build/standalone.gypi:1012: '-std=gnu++11' ], what about the other places in this file where we have gnu++0x?
https://codereview.chromium.org/1637473003/diff/120001/build/standalone.gypi File build/standalone.gypi (right): https://codereview.chromium.org/1637473003/diff/120001/build/standalone.gypi#... build/standalone.gypi:1012: '-std=gnu++11' ], On 2016/02/01 10:06:46, jochen wrote: > what about the other places in this file where we have gnu++0x? For this CL, how about using -std=c++11, see patch 7.
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1637473003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1637473003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mhm so you'll change all the other -std=gnu++0x in another CL? I guess we should stick with gnu++11 as chromium does
On 2016/02/01 12:05:04, jochen wrote: > mhm > > so you'll change all the other -std=gnu++0x in another CL? > > I guess we should stick with gnu++11 as chromium does Fine. See patch 8. I keep it as it is in chromium right now and switch later when crbug.com/427584 gets resolved. I wouldn't touch other places now in this android-specific CL. I can prep another CL for switching the other flags.
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1637473003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1637473003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/1637473003/diff/160001/build/standalone.gypi File build/standalone.gypi (right): https://codereview.chromium.org/1637473003/diff/160001/build/standalone.gypi#... build/standalone.gypi:1046: '-Wl,-rpath-link=<(android_lib)', # FIXME: what is this for? what exactly do you mean?
https://codereview.chromium.org/1637473003/diff/160001/build/standalone.gypi File build/standalone.gypi (right): https://codereview.chromium.org/1637473003/diff/160001/build/standalone.gypi#... build/standalone.gypi:1046: '-Wl,-rpath-link=<(android_lib)', # FIXME: what is this for? On 2016/02/03 13:42:17, jochen wrote: > what exactly do you mean? Referring to Nico as this was copied from his patch.
https://codereview.chromium.org/1637473003/diff/160001/build/standalone.gypi File build/standalone.gypi (right): https://codereview.chromium.org/1637473003/diff/160001/build/standalone.gypi#... build/standalone.gypi:1046: '-Wl,-rpath-link=<(android_lib)', # FIXME: what is this for? On 2016/02/03 14:07:56, Michael Achenbach wrote: > On 2016/02/03 13:42:17, jochen wrote: > > what exactly do you mean? > > Referring to Nico as this was copied from his patch. Chromium doesn't have this. Maybe you can remove this line?
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1637473003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1637473003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1637473003/diff/160001/build/standalone.gypi File build/standalone.gypi (right): https://codereview.chromium.org/1637473003/diff/160001/build/standalone.gypi#... build/standalone.gypi:1046: '-Wl,-rpath-link=<(android_lib)', # FIXME: what is this for? On 2016/02/03 14:26:01, Nico wrote: > On 2016/02/03 14:07:56, Michael Achenbach wrote: > > On 2016/02/03 13:42:17, jochen wrote: > > > what exactly do you mean? > > > > Referring to Nico as this was copied from his patch. > > Chromium doesn't have this. Maybe you can remove this line? I removed it...
No objections, so I'll land this now...
The CQ bit was checked by machenbach@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1637473003/#ps180001 (title: "Remove unnecessary ldflags.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1637473003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1637473003/180001
Message was sent while issue was closed.
Description was changed from ========== android: Use libc++ instead of stlport. Useful for example for using atomicops_internal_portable.h on Android. BUG=v8:4615 LOG=y patch from issue 1525813002 at patchset 1 (http://crrev.com/1525813002#ps1) ========== to ========== android: Use libc++ instead of stlport. Useful for example for using atomicops_internal_portable.h on Android. BUG=v8:4615 LOG=y patch from issue 1525813002 at patchset 1 (http://crrev.com/1525813002#ps1) ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== android: Use libc++ instead of stlport. Useful for example for using atomicops_internal_portable.h on Android. BUG=v8:4615 LOG=y patch from issue 1525813002 at patchset 1 (http://crrev.com/1525813002#ps1) ========== to ========== android: Use libc++ instead of stlport. Useful for example for using atomicops_internal_portable.h on Android. BUG=v8:4615 LOG=y patch from issue 1525813002 at patchset 1 (http://crrev.com/1525813002#ps1) Committed: https://crrev.com/ee57e14c6f43e40d097456f23e6af8c27ef35e34 Cr-Commit-Position: refs/heads/master@{#33804} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/ee57e14c6f43e40d097456f23e6af8c27ef35e34 Cr-Commit-Position: refs/heads/master@{#33804} |
