Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1326053003:
port cfi to gn (Closed)
DescriptionPort CFI build configuration to GN.
BUG=464797
R=brettw@chromium.org
Committed: https://crrev.com/34ab19d59ff4a5bc098d90eb7c4fc76afec568ef
Cr-Commit-Position: refs/heads/master@{#348323}
Patch Set 1 #Patch Set 2 : Disable CFI when targeting nacl #
Total comments: 4
Patch Set 3 : Move CFI config to sanitizers.gni #Patch Set 4 : Revert gn changes; start using toolchain_os; fix use_cfi_diag=true #
Total comments: 2
Patch Set 5 : Rephrase comment #
Messages
Total messages: 15 (4 generated)
brettw@chromium.org changed reviewers: + brettw@chromium.org
Thanks for the patch. Long comments, sorry! https://codereview.chromium.org/1326053003/diff/20001/build/config/BUILDCONFI... File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/1326053003/diff/20001/build/config/BUILDCONFI... build/config/BUILDCONFIG.gn:150: is_cfi = false I was thinking of removing the *san flags from here (trying to keep the global file small). Doing this will require refactoring the symbol configuration a bit that I was procrastinating on. But even more sanitizers makes me realize we really need to do this. In the mean time, can you move your flags to build/config/sanitizers/sanitizers.gni and import that file when you need to use the flags? Until I clean up the symbols a CFI build will presumably need symbol_level=1 set to work properly (assuming you really do need this level of symbols). You can add an assert in the sanitizers.gni file that this is the case (maybe you can also tolerate 2?) so people don't screw up. Then you can do it with no changes to this file. https://codereview.chromium.org/1326053003/diff/20001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/1326053003/diff/20001/build/config/compiler/B... build/config/compiler/BUILD.gn:258: arflags += [ My initial reaction is "I don't want this" but if we need it, I guess adding something like this is fine. Can you give me some background about why this is needed? I though "ar" basically cat-s the files together, so I'm not sure why plugin would be necessary or what it could even do. A second question would be whether we would definitely want this applied to all "ar" commands regardless of target. If that's the case, we can more simply make this change by just adding these flags to the command that's executed when we do an "alink". To try this, look in build/toolchain/gcc_toolchain.gni in the "alink" step and just append the flags to the ar command for the toolchain definitions in build/toolchain/linux/BUILD.gn If we do need to add arflags to gn, we'll have to do it in three phases, the GN changes, push the binary, then check in the build changes.
https://codereview.chromium.org/1326053003/diff/20001/build/config/BUILDCONFI... File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/1326053003/diff/20001/build/config/BUILDCONFI... build/config/BUILDCONFIG.gn:150: is_cfi = false On 2015/09/03 04:41:20, brettw wrote: > I was thinking of removing the *san flags from here (trying to keep the global > file small). Doing this will require refactoring the symbol configuration a bit > that I was procrastinating on. But even more sanitizers makes me realize we > really need to do this. > > In the mean time, can you move your flags to > build/config/sanitizers/sanitizers.gni and import that file when you need to use > the flags? Done. > Until I clean up the symbols a CFI build will presumably need symbol_level=1 set > to work properly (assuming you really do need this level of symbols). You can > add an assert in the sanitizers.gni file that this is the case (maybe you can > also tolerate 2?) so people don't screw up. The CFI build does not require symbols other than for better stack traces, so the default is probably fine. https://codereview.chromium.org/1326053003/diff/20001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/1326053003/diff/20001/build/config/compiler/B... build/config/compiler/BUILD.gn:258: arflags += [ On 2015/09/03 04:41:20, brettw wrote: > My initial reaction is "I don't want this" but if we need it, I guess adding > something like this is fine. > > Can you give me some background about why this is needed? I though "ar" > basically cat-s the files together, so I'm not sure why plugin would be > necessary or what it could even do. Archive files contain a symbol table for each object in the archive. If the inputs are LTO objects, the plugin is needed to build the symbol table. > A second question would be whether we would definitely want this applied to all > "ar" commands regardless of target. If that's the case, we can more simply make > this change by just adding these flags to the command that's executed when we do > an "alink". > > To try this, look in build/toolchain/gcc_toolchain.gni in the "alink" step and > just append the flags to the ar command for the toolchain definitions in > build/toolchain/linux/BUILD.gn There's the complication that we don't want these flags to appear in NaCl targets at least for now (mainly because the NaCl threat model is different, but also for the more practical reason that the NaCl toolchain was built without LTO plugin support, so passing the --plugin flag fails for NaCl archives). I couldn't get the flags to be different for NaCl targets by adding if statements within the alink block. Specifically, this didn't help (I also tried is_nacl): diff --git a/build/toolchain/gcc_toolchain.gni b/build/toolchain/gcc_toolchain.gni index 924bf1b..79d171b 100644 --- a/build/toolchain/gcc_toolchain.gni +++ b/build/toolchain/gcc_toolchain.gni @@ -3,6 +3,7 @@ # found in the LICENSE file. import("//build/toolchain/toolchain.gni") +import("//build/config/sanitizers/sanitizers.gni") # This value will be inherited in the toolchain below. concurrent_links = exec_script("get_concurrent_links.py", [], "value") @@ -173,7 +174,14 @@ template("gcc_toolchain") { tool("alink") { rspfile = "{{output}}.rsp" - command = "rm -f {{output}} && $ar rcs {{arflags}} {{output}} @$rspfile" + arflags = "" + if (is_cfi && current_os != "nacl") { + gold_plugin_path = rebase_path( + "//third_party/llvm-build/Release+Asserts/lib/LLVMgold.so", + root_build_dir) + arflags = "--plugin $gold_plugin_path" + } + command = "rm -f {{output}} && $ar rcs $arflags {{output}} @$rspfile" description = "AR {{output}}" rspfile_content = "{{inputs}}" outputs = [ > If we do need to add arflags to gn, we'll have to do it in three phases, the GN > changes, push the binary, then check in the build changes. I suppose this is how we'll have to proceed, correct?
On 2015/09/04 21:57:00, pcc1 wrote: > https://codereview.chromium.org/1326053003/diff/20001/build/config/BUILDCONFI... > File build/config/BUILDCONFIG.gn (right): > > https://codereview.chromium.org/1326053003/diff/20001/build/config/BUILDCONFI... > build/config/BUILDCONFIG.gn:150: is_cfi = false > On 2015/09/03 04:41:20, brettw wrote: > > I was thinking of removing the *san flags from here (trying to keep the global > > file small). Doing this will require refactoring the symbol configuration a > bit > > that I was procrastinating on. But even more sanitizers makes me realize we > > really need to do this. > > > > In the mean time, can you move your flags to > > build/config/sanitizers/sanitizers.gni and import that file when you need to > use > > the flags? > > Done. > > > Until I clean up the symbols a CFI build will presumably need symbol_level=1 > set > > to work properly (assuming you really do need this level of symbols). You can > > add an assert in the sanitizers.gni file that this is the case (maybe you can > > also tolerate 2?) so people don't screw up. > > The CFI build does not require symbols other than for better stack traces, so > the default is probably fine. > > https://codereview.chromium.org/1326053003/diff/20001/build/config/compiler/B... > File build/config/compiler/BUILD.gn (right): > > https://codereview.chromium.org/1326053003/diff/20001/build/config/compiler/B... > build/config/compiler/BUILD.gn:258: arflags += [ > On 2015/09/03 04:41:20, brettw wrote: > > My initial reaction is "I don't want this" but if we need it, I guess adding > > something like this is fine. > > > > Can you give me some background about why this is needed? I though "ar" > > basically cat-s the files together, so I'm not sure why plugin would be > > necessary or what it could even do. > > Archive files contain a symbol table for each object in the archive. If the > inputs are LTO objects, the plugin is needed to build the symbol table. > > > A second question would be whether we would definitely want this applied to > all > > "ar" commands regardless of target. If that's the case, we can more simply > make > > this change by just adding these flags to the command that's executed when we > do > > an "alink". > > > > To try this, look in build/toolchain/gcc_toolchain.gni in the "alink" step and > > just append the flags to the ar command for the toolchain definitions in > > build/toolchain/linux/BUILD.gn > > There's the complication that we don't want these flags to appear in NaCl > targets at least for now (mainly because the NaCl threat model is different, but > also for the more practical reason that the NaCl toolchain was built without LTO > plugin support, so passing the --plugin flag fails for NaCl archives). I > couldn't get the flags to be different for NaCl targets by adding if statements > within the alink block. Specifically, this didn't help (I also tried is_nacl): > > diff --git a/build/toolchain/gcc_toolchain.gni > b/build/toolchain/gcc_toolchain.gni > index 924bf1b..79d171b 100644 > --- a/build/toolchain/gcc_toolchain.gni > +++ b/build/toolchain/gcc_toolchain.gni > @@ -3,6 +3,7 @@ > # found in the LICENSE file. > > import("//build/toolchain/toolchain.gni") > +import("//build/config/sanitizers/sanitizers.gni") > > # This value will be inherited in the toolchain below. > concurrent_links = exec_script("get_concurrent_links.py", [], "value") > @@ -173,7 +174,14 @@ template("gcc_toolchain") { > > tool("alink") { > rspfile = "{{output}}.rsp" > - command = "rm -f {{output}} && $ar rcs {{arflags}} {{output}} @$rspfile" > + arflags = "" > + if (is_cfi && current_os != "nacl") { Sorry this is a bit confusing. I've been wanting to redo how some of this works so your code would be correct. For now, I think it should work if you write "toolchain_os" instead of "current_os" (basically because this BUILD file is evaluated once to define the toolchains, the "current os" isn't the same as the one you're defining the toolchain for.
On 2015/09/10 18:11:45, brettw wrote: > On 2015/09/04 21:57:00, pcc1 wrote: > > > https://codereview.chromium.org/1326053003/diff/20001/build/config/BUILDCONFI... > > File build/config/BUILDCONFIG.gn (right): > > > > > https://codereview.chromium.org/1326053003/diff/20001/build/config/BUILDCONFI... > > build/config/BUILDCONFIG.gn:150: is_cfi = false > > On 2015/09/03 04:41:20, brettw wrote: > > > I was thinking of removing the *san flags from here (trying to keep the > global > > > file small). Doing this will require refactoring the symbol configuration a > > bit > > > that I was procrastinating on. But even more sanitizers makes me realize we > > > really need to do this. > > > > > > In the mean time, can you move your flags to > > > build/config/sanitizers/sanitizers.gni and import that file when you need to > > use > > > the flags? > > > > Done. > > > > > Until I clean up the symbols a CFI build will presumably need symbol_level=1 > > set > > > to work properly (assuming you really do need this level of symbols). You > can > > > add an assert in the sanitizers.gni file that this is the case (maybe you > can > > > also tolerate 2?) so people don't screw up. > > > > The CFI build does not require symbols other than for better stack traces, so > > the default is probably fine. > > > > > https://codereview.chromium.org/1326053003/diff/20001/build/config/compiler/B... > > File build/config/compiler/BUILD.gn (right): > > > > > https://codereview.chromium.org/1326053003/diff/20001/build/config/compiler/B... > > build/config/compiler/BUILD.gn:258: arflags += [ > > On 2015/09/03 04:41:20, brettw wrote: > > > My initial reaction is "I don't want this" but if we need it, I guess adding > > > something like this is fine. > > > > > > Can you give me some background about why this is needed? I though "ar" > > > basically cat-s the files together, so I'm not sure why plugin would be > > > necessary or what it could even do. > > > > Archive files contain a symbol table for each object in the archive. If the > > inputs are LTO objects, the plugin is needed to build the symbol table. > > > > > A second question would be whether we would definitely want this applied to > > all > > > "ar" commands regardless of target. If that's the case, we can more simply > > make > > > this change by just adding these flags to the command that's executed when > we > > do > > > an "alink". > > > > > > To try this, look in build/toolchain/gcc_toolchain.gni in the "alink" step > and > > > just append the flags to the ar command for the toolchain definitions in > > > build/toolchain/linux/BUILD.gn > > > > There's the complication that we don't want these flags to appear in NaCl > > targets at least for now (mainly because the NaCl threat model is different, > but > > also for the more practical reason that the NaCl toolchain was built without > LTO > > plugin support, so passing the --plugin flag fails for NaCl archives). I > > couldn't get the flags to be different for NaCl targets by adding if > statements > > within the alink block. Specifically, this didn't help (I also tried is_nacl): > > > > diff --git a/build/toolchain/gcc_toolchain.gni > > b/build/toolchain/gcc_toolchain.gni > > index 924bf1b..79d171b 100644 > > --- a/build/toolchain/gcc_toolchain.gni > > +++ b/build/toolchain/gcc_toolchain.gni > > @@ -3,6 +3,7 @@ > > # found in the LICENSE file. > > > > import("//build/toolchain/toolchain.gni") > > +import("//build/config/sanitizers/sanitizers.gni") > > > > # This value will be inherited in the toolchain below. > > concurrent_links = exec_script("get_concurrent_links.py", [], "value") > > @@ -173,7 +174,14 @@ template("gcc_toolchain") { > > > > tool("alink") { > > rspfile = "{{output}}.rsp" > > - command = "rm -f {{output}} && $ar rcs {{arflags}} {{output}} > @$rspfile" > > + arflags = "" > > + if (is_cfi && current_os != "nacl") { > > Sorry this is a bit confusing. I've been wanting to redo how some of this works > so your code would be correct. For now, I think it should work if you write > "toolchain_os" instead of "current_os" (basically because this BUILD file is > evaluated once to define the toolchains, the "current os" isn't the same as the > one you're defining the toolchain for. Thanks, that was the hint I needed to get this working. I'm now using invoker.toolchain_os and have reverted the gn changes.
https://codereview.chromium.org/1326053003/diff/60001/build/config/sanitizers... File build/config/sanitizers/sanitizers.gni (right): https://codereview.chromium.org/1326053003/diff/60001/build/config/sanitizers... build/config/sanitizers/sanitizers.gni:30: # Whether Control Flow Integrity prints diagnostics instead of crashing. Can you rephrase this comment to make more clear what happens in the true vs. false case?
lgtm with the above change.
pcc@google.com changed reviewers: + pcc@google.com
https://codereview.chromium.org/1326053003/diff/60001/build/config/sanitizers... File build/config/sanitizers/sanitizers.gni (right): https://codereview.chromium.org/1326053003/diff/60001/build/config/sanitizers... build/config/sanitizers/sanitizers.gni:30: # Whether Control Flow Integrity prints diagnostics instead of crashing. On 2015/09/10 23:16:34, brettw wrote: > Can you rephrase this comment to make more clear what happens in the true vs. > false case? Done.
The CQ bit was checked by pcc@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1326053003/#ps80001 (title: "Rephrase comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1326053003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1326053003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/34ab19d59ff4a5bc098d90eb7c4fc76afec568ef Cr-Commit-Position: refs/heads/master@{#348323}
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/34ab19d59ff4a5bc098d90eb7c4fc76afec568ef Cr-Commit-Position: refs/heads/master@{#348323} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
