Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1145333004:
Add GN targets for libc++ and libc++abi. (Closed)
|
Created:
5 years, 7 months ago by Sam McNally Modified:
5 years, 6 months ago CC:
chrome-apps-syd-reviews_chromium.org, earthdok Base URL:
https://chromium.googlesource.com/chromium/buildtools.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd GN targets for libc++ and libc++abi.
R=earthdok@chromium.org
Committed: fa660d47fa1a6c649d5c29e001348447c55709e6
Patch Set 1 : #
Total comments: 9
Patch Set 2 : #Patch Set 3 : #
Total comments: 8
Patch Set 4 : #Messages
Total messages: 15 (6 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
sammc@chromium.org changed reviewers: + glider@chromium.org
This is a dependency of https://codereview.chromium.org/1158643002/.
https://codereview.chromium.org/1145333004/diff/40001/third_party/libc++/BUIL... File third_party/libc++/BUILD.gn (right): https://codereview.chromium.org/1145333004/diff/40001/third_party/libc++/BUIL... third_party/libc++/BUILD.gn:89: "-Wl,-R,\$ORIGIN/", Does the above comment apply to GN?
earthdok@chromium.org changed reviewers: + earthdok@chromium.org
https://codereview.chromium.org/1145333004/diff/40001/third_party/libc++/BUIL... File third_party/libc++/BUILD.gn (right): https://codereview.chromium.org/1145333004/diff/40001/third_party/libc++/BUIL... third_party/libc++/BUILD.gn:46: "-fstrict-aliasing", Why are some flags passed explicitly while others are implemented as build configs? https://codereview.chromium.org/1145333004/diff/40001/third_party/libc++/BUIL... third_party/libc++/BUILD.gn:51: "//build/config/gcc:no_exceptions", Will those work with clang? https://codereview.chromium.org/1145333004/diff/40001/third_party/libc++/BUIL... third_party/libc++/BUILD.gn:89: "-Wl,-R,\$ORIGIN/", On 2015/05/25 08:30:09, Alexander Potapenko wrote: > Does the above comment apply to GN? The security issue here is that a setuid executable (chrome-sandbox) gets a relative RPATH. In issue 315, this happens due to a bug in GYP. Here, however, the same problem is caused by our own LDFLAGS, regardless of the build system. The comment should probably be reworded to emphasize that issue 315 is not the cause, but another example of the same issue. Also we may want to disable libc++ for chrome-sandbox somehow. https://codereview.chromium.org/1145333004/diff/40001/third_party/libc++abi/B... File third_party/libc++abi/BUILD.gn (right): https://codereview.chromium.org/1145333004/diff/40001/third_party/libc++abi/B... third_party/libc++abi/BUILD.gn:29: cflags = [ What happened to the exclusion lists? 'cflags_cc!': [ '-fno-exceptions', '-fno-rtti', ], 'cflags!': [ '-fvisibility=hidden', ], 'ldflags!': [ '-pthread', ],
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/1145333004/diff/40001/third_party/libc++/BUIL... File third_party/libc++/BUILD.gn (right): https://codereview.chromium.org/1145333004/diff/40001/third_party/libc++/BUIL... third_party/libc++/BUILD.gn:46: "-fstrict-aliasing", On 2015/05/25 15:48:19, earthdok wrote: > Why are some flags passed explicitly while others are implemented as build > configs? The others were controlled by existing configs. Added a config for the remaining options to be shared by this and libc++abi. https://codereview.chromium.org/1145333004/diff/40001/third_party/libc++/BUIL... third_party/libc++/BUILD.gn:51: "//build/config/gcc:no_exceptions", On 2015/05/25 15:48:19, earthdok wrote: > Will those work with clang? Yes. https://codereview.chromium.org/1145333004/diff/40001/third_party/libc++/BUIL... third_party/libc++/BUILD.gn:89: "-Wl,-R,\$ORIGIN/", On 2015/05/25 15:48:19, earthdok wrote: > On 2015/05/25 08:30:09, Alexander Potapenko wrote: > > Does the above comment apply to GN? > > The security issue here is that a setuid executable (chrome-sandbox) gets a > relative RPATH. In issue 315, this happens due to a bug in GYP. Here, however, > the same problem is caused by our own LDFLAGS, regardless of the build system. > > The comment should probably be reworded to emphasize that issue 315 is not the > cause, but another example of the same issue. Also we may want to disable libc++ > for chrome-sandbox somehow. Done. https://codereview.chromium.org/1145333004/diff/40001/third_party/libc++abi/B... File third_party/libc++abi/BUILD.gn (right): https://codereview.chromium.org/1145333004/diff/40001/third_party/libc++abi/B... third_party/libc++abi/BUILD.gn:29: cflags = [ On 2015/05/25 15:48:19, earthdok wrote: > What happened to the exclusion lists? > > 'cflags_cc!': [ > '-fno-exceptions', > '-fno-rtti', > ], > 'cflags!': [ > '-fvisibility=hidden', > ], > 'ldflags!': [ > '-pthread', > ], The cflags and cflags_cc are handled by removing configs. I can't figure out a way to remove -pthread from ldflags. Configs and targets can't remove flags set by other configs; targets can only remove entire configs. -pthread is set by the //build/config/compiler config, which is probably one we want to keep.
https://codereview.chromium.org/1145333004/diff/100001/third_party/libc++/BUI... File third_party/libc++/BUILD.gn (right): https://codereview.chromium.org/1145333004/diff/100001/third_party/libc++/BUI... third_party/libc++/BUILD.gn:70: "-Wl,-R,\$ORIGIN/", $ORIGIN/lib? https://codereview.chromium.org/1145333004/diff/100001/third_party/libc++abi/... File third_party/libc++abi/BUILD.gn (right): https://codereview.chromium.org/1145333004/diff/100001/third_party/libc++abi/... third_party/libc++abi/BUILD.gn:5: config("config") { If you want to have a common config then please move it to libc++/BUILD.gn. https://codereview.chromium.org/1145333004/diff/100001/third_party/libc++abi/... third_party/libc++abi/BUILD.gn:7: "trunk/include", What is this path relative to? I.e., when libc++/BUILD.gn includes this config, will "trunk/include" evaluate to "libc++abi/trunk/include" or "libc++/trunk/include"? https://codereview.chromium.org/1145333004/diff/100001/third_party/libc++abi/... third_party/libc++abi/BUILD.gn:19: # TODO(earthdok): find out what's causing the warning. I'm resolving this TODO in https://codereview.chromium.org/1154213002/ It also contains a few minor flag changes.
Patchset #4 (id:120001) has been deleted
https://codereview.chromium.org/1145333004/diff/100001/third_party/libc++/BUI... File third_party/libc++/BUILD.gn (right): https://codereview.chromium.org/1145333004/diff/100001/third_party/libc++/BUI... third_party/libc++/BUILD.gn:70: "-Wl,-R,\$ORIGIN/", On 2015/05/26 16:18:46, earthdok wrote: > $ORIGIN/lib? GN puts shared libraries in out/Foo instead of out/Foo/lib. https://codereview.chromium.org/1145333004/diff/100001/third_party/libc++abi/... File third_party/libc++abi/BUILD.gn (right): https://codereview.chromium.org/1145333004/diff/100001/third_party/libc++abi/... third_party/libc++abi/BUILD.gn:5: config("config") { On 2015/05/26 16:18:46, earthdok wrote: > If you want to have a common config then please move it to libc++/BUILD.gn. Done. https://codereview.chromium.org/1145333004/diff/100001/third_party/libc++abi/... third_party/libc++abi/BUILD.gn:7: "trunk/include", On 2015/05/26 16:18:46, earthdok wrote: > What is this path relative to? I.e., when libc++/BUILD.gn includes this config, > will "trunk/include" evaluate to "libc++abi/trunk/include" or > "libc++/trunk/include"? Relative to the BUILD.gn containing the config. While the config was in this file "trunk/include" would always evaluate to "libc++abi/trunk/include", no matter what depends on the config. https://codereview.chromium.org/1145333004/diff/100001/third_party/libc++abi/... third_party/libc++abi/BUILD.gn:19: # TODO(earthdok): find out what's causing the warning. On 2015/05/26 16:18:46, earthdok wrote: > I'm resolving this TODO in https://codereview.chromium.org/1154213002/ It also > contains a few minor flag changes. Done.
lgtm
Message was sent while issue was closed.
Committed patchset #4 (id:140001) manually as fa660d47fa1a6c649d5c29e001348447c55709e6 (presubmit successful). |
