Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2049033004:
Fix NaCl executable linkages under ASAN. (Closed)
|
Created:
4 years, 6 months ago by Dirk Pranke Modified:
4 years, 6 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/src/native_client.git@master Target Ref:
refs/heads/master Project:
nacl Visibility:
Public. |
DescriptionFix NaCL executable linkages under ASAN.
When linking binaries for use in the sanitizers, every executable needs
to depend on a given library. We don't have a general way of enforcing this in GN, and most binaries pick this up through the test() template, but there were a few binaries that weren't tests that needed this set explicitly.
R=thakis@chromium.org, bradnelson@chromium.org
BUG=618450
Committed: https://chromium.googlesource.com/native_client/src/native_client/+/5152c12c447761d64d70056104d0b5037830415a
Patch Set 1 #
Total comments: 1
Patch Set 2 : add deps to sel_ldr, ncval_new as well #
Total comments: 3
Messages
Total messages: 18 (4 generated)
lgtm i suppose https://codereview.chromium.org/2049033004/diff/1/src/trusted/debug_stub/BUIL... File src/trusted/debug_stub/BUILD.gn (right): https://codereview.chromium.org/2049033004/diff/1/src/trusted/debug_stub/BUIL... src/trusted/debug_stub/BUILD.gn:54: "//build/config/sanitizers:deps", huh, i thought your CL: https://codereview.chromium.org/2031903002/ made it so that instrumented_libraries:prebuilt_link_helper is a dep of all executables which I thought did this. If it didn't, this probably isn't the only executable that needs this (? – did you do a local build of 'all'?)
On 2016/06/08 23:38:45, Nico wrote: > lgtm i suppose > > https://codereview.chromium.org/2049033004/diff/1/src/trusted/debug_stub/BUIL... > File src/trusted/debug_stub/BUILD.gn (right): > > https://codereview.chromium.org/2049033004/diff/1/src/trusted/debug_stub/BUIL... > src/trusted/debug_stub/BUILD.gn:54: "//build/config/sanitizers:deps", > huh, i thought your CL: https://codereview.chromium.org/2031903002/ made it so > that instrumented_libraries:prebuilt_link_helper is a dep of all executables > which I thought did this. That CL fixed the issue for every executable that *did* specify the deps, but as I explained in the bug, there's no way to make every executable have a set of deps automatically (unlike flags, which can be done via the default configs). > If it didn't, this probably isn't the only executable > that needs this (? – did you do a local build of 'all'?) I am doing a local build now of all to see what else is broken. It's ... not fast. As I said in the bug, I can't promise that I'll be able to fix every executable in a timely manner, but it depends on how many are missing.
On 2016/06/08 23:44:45, Dirk Pranke wrote: > On 2016/06/08 23:38:45, Nico wrote: > > lgtm i suppose > > > > > https://codereview.chromium.org/2049033004/diff/1/src/trusted/debug_stub/BUIL... > > File src/trusted/debug_stub/BUILD.gn (right): > > > > > https://codereview.chromium.org/2049033004/diff/1/src/trusted/debug_stub/BUIL... > > src/trusted/debug_stub/BUILD.gn:54: "//build/config/sanitizers:deps", > > huh, i thought your CL: https://codereview.chromium.org/2031903002/ made it so > > that instrumented_libraries:prebuilt_link_helper is a dep of all executables > > which I thought did this. > > That CL fixed the issue for every executable that *did* specify the deps, but as > I > explained in the bug, there's no way to make every executable have a set of deps > automatically (unlike flags, which can be done via the default configs). I didn't read the bug, but it'd be good if there was some way to say "if asan, make every executable depend on foo" imho. > > > If it didn't, this probably isn't the only executable > > that needs this (? – did you do a local build of 'all'?) > > I am doing a local build now of all to see what else is broken. It's ... not > fast. As > I said in the bug, I can't promise that I'll be able to fix every executable in > a timely > manner, but it depends on how many are missing.
On 2016/06/08 23:46:58, Nico wrote: > On 2016/06/08 23:44:45, Dirk Pranke wrote: > > On 2016/06/08 23:38:45, Nico wrote: > > > lgtm i suppose > > > > > > > > > https://codereview.chromium.org/2049033004/diff/1/src/trusted/debug_stub/BUIL... > > > File src/trusted/debug_stub/BUILD.gn (right): > > > > > > > > > https://codereview.chromium.org/2049033004/diff/1/src/trusted/debug_stub/BUIL... > > > src/trusted/debug_stub/BUILD.gn:54: "//build/config/sanitizers:deps", > > > huh, i thought your CL: https://codereview.chromium.org/2031903002/ made it > so > > > that instrumented_libraries:prebuilt_link_helper is a dep of all executables > > > which I thought did this. > > > > That CL fixed the issue for every executable that *did* specify the deps, but > as > > I > > explained in the bug, there's no way to make every executable have a set of > deps > > automatically (unlike flags, which can be done via the default configs). > > I didn't read the bug, but it'd be good if there was some way to say "if asan, > make every executable depend on foo" imho. What's the point in filing bugs if you don't actually read the comments on them? In theory, your idea seems plausible. In practice, this can be a little dodgy since executables can be dependencies of other executables, and GN would have to be smart enough to detect (and reject) cycles. It does this in the normal case, of course, but I'm not sure how hard it would be to do this for the default configs or default deps cases. There's relatively few bare executables in the GN build; most of them are covered by the test() template, which adds this config automatically. At any rate, debating whether GN should have this functionality or not is well out of the scope of this particular bug or fix.
On 2016/06/08 23:54:07, Dirk Pranke wrote: > On 2016/06/08 23:46:58, Nico wrote: > > On 2016/06/08 23:44:45, Dirk Pranke wrote: > > > On 2016/06/08 23:38:45, Nico wrote: > > > > lgtm i suppose > > > > > > > > > > > > > > https://codereview.chromium.org/2049033004/diff/1/src/trusted/debug_stub/BUIL... > > > > File src/trusted/debug_stub/BUILD.gn (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2049033004/diff/1/src/trusted/debug_stub/BUIL... > > > > src/trusted/debug_stub/BUILD.gn:54: "//build/config/sanitizers:deps", > > > > huh, i thought your CL: https://codereview.chromium.org/2031903002/ made > it > > so > > > > that instrumented_libraries:prebuilt_link_helper is a dep of all > executables > > > > which I thought did this. > > > > > > That CL fixed the issue for every executable that *did* specify the deps, > but > > as > > > I > > > explained in the bug, there's no way to make every executable have a set of > > deps > > > automatically (unlike flags, which can be done via the default configs). > > > > I didn't read the bug, but it'd be good if there was some way to say "if asan, > > make every executable depend on foo" imho. > > What's the point in filing bugs if you don't actually read the comments on them? I've read it now. 'twas just me going through mails newest-to-oldest :-) > In theory, your idea seems plausible. In practice, this can be a little dodgy > since > executables can be dependencies of other executables, and GN would have to be > smart enough to detect (and reject) cycles. It does this in the normal case, > of course, but I'm not sure how hard it would be to do this for the default > configs > or default deps cases. > > There's relatively few bare executables in the GN build; most of them are > covered > by the test() template, which adds this config automatically. At any rate, > debating > whether GN should have this functionality or not is well out of the scope of > this > particular bug or fix.
On 2016/06/08 23:55:53, Nico wrote: > On 2016/06/08 23:54:07, Dirk Pranke wrote: > > On 2016/06/08 23:46:58, Nico wrote: > > > On 2016/06/08 23:44:45, Dirk Pranke wrote: > > > > On 2016/06/08 23:38:45, Nico wrote: > > > > > lgtm i suppose > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2049033004/diff/1/src/trusted/debug_stub/BUIL... > > > > > File src/trusted/debug_stub/BUILD.gn (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2049033004/diff/1/src/trusted/debug_stub/BUIL... > > > > > src/trusted/debug_stub/BUILD.gn:54: "//build/config/sanitizers:deps", > > > > > huh, i thought your CL: https://codereview.chromium.org/2031903002/ made > > it > > > so > > > > > that instrumented_libraries:prebuilt_link_helper is a dep of all > > executables > > > > > which I thought did this. > > > > > > > > That CL fixed the issue for every executable that *did* specify the deps, > > but > > > as > > > > I > > > > explained in the bug, there's no way to make every executable have a set > of > > > deps > > > > automatically (unlike flags, which can be done via the default configs). > > > > > > I didn't read the bug, but it'd be good if there was some way to say "if > asan, > > > make every executable depend on foo" imho. > > > > What's the point in filing bugs if you don't actually read the comments on > them? > > I've read it now. 'twas just me going through mails newest-to-oldest :-) > > > In theory, your idea seems plausible. In practice, this can be a little dodgy > > since > > executables can be dependencies of other executables, and GN would have to be > > smart enough to detect (and reject) cycles. It does this in the normal case, > > of course, but I'm not sure how hard it would be to do this for the default > > configs > > or default deps cases. > > > > There's relatively few bare executables in the GN build; most of them are > > covered > > by the test() template, which adds this config automatically. At any rate, > > debating > > whether GN should have this functionality or not is well out of the scope of > > this > > particular bug or fix. Also, brettw@ reminded me that we could set the default deps for executables, but if we did, then everyone would have to write `deps += [...]` instead of `deps = [...]` for their targets (like you need to do for configs, because we have default configs), but we felt that that would seem awkward and unintuitive since most of the time there are no default deps (and there are default configs).
Description was changed from ========== Fix gdb_rsp_unittest link under ASAN. R=thakis@chromium.org, bradnelson@chromium.org BUG=618450 ========== to ========== Fix NaCL executable linkages under ASAN. When linking binaries for use in the sanitizers, every executable needs to depend on a given library. We don't have a general way of enforcing this in GN, and most binaries pick this up through the test() template, but there were a few binaries that weren't tests that needed this set explicitly. R=thakis@chromium.org, bradnelson@chromium.org BUG=618450 ==========
Okay, with the other two binaries fixed, `all` built for me locally using the same flags the bot uses.
lgtm, that ain't so bad :-) https://codereview.chromium.org/2049033004/diff/20001/src/trusted/validator/d... File src/trusted/validator/driver/BUILD.gn (right): https://codereview.chromium.org/2049033004/diff/20001/src/trusted/validator/d... src/trusted/validator/driver/BUILD.gn:5: # Autogenerated from src/trusted/validator/driver/build.scons. does this mean "do not edit"? not clear
https://codereview.chromium.org/2049033004/diff/20001/src/trusted/validator/d... File src/trusted/validator/driver/BUILD.gn (right): https://codereview.chromium.org/2049033004/diff/20001/src/trusted/validator/d... src/trusted/validator/driver/BUILD.gn:5: # Autogenerated from src/trusted/validator/driver/build.scons. On 2016/06/09 00:41:18, Nico wrote: > does this mean "do not edit"? not clear Hm. Good question. Hopefully bradnelson@ can answer.
bradnelson@google.com changed reviewers: + bradnelson@google.com
https://codereview.chromium.org/2049033004/diff/20001/src/trusted/validator/d... File src/trusted/validator/driver/BUILD.gn (right): https://codereview.chromium.org/2049033004/diff/20001/src/trusted/validator/d... src/trusted/validator/driver/BUILD.gn:5: # Autogenerated from src/trusted/validator/driver/build.scons. On 2016/06/09 01:06:14, Dirk Pranke wrote: > On 2016/06/09 00:41:18, Nico wrote: > > does this mean "do not edit"? not clear > > Hm. Good question. Hopefully bradnelson@ can answer. That's one from noelallen's conversion script. It's fine to change (it was a one time conversion).
The CQ bit was checked by bradnelson@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049033004/20001
Message was sent while issue was closed.
Description was changed from ========== Fix NaCL executable linkages under ASAN. When linking binaries for use in the sanitizers, every executable needs to depend on a given library. We don't have a general way of enforcing this in GN, and most binaries pick this up through the test() template, but there were a few binaries that weren't tests that needed this set explicitly. R=thakis@chromium.org, bradnelson@chromium.org BUG=618450 ========== to ========== Fix NaCL executable linkages under ASAN. When linking binaries for use in the sanitizers, every executable needs to depend on a given library. We don't have a general way of enforcing this in GN, and most binaries pick this up through the test() template, but there were a few binaries that weren't tests that needed this set explicitly. R=thakis@chromium.org, bradnelson@chromium.org BUG=618450 Committed: https://chromium.googlesource.com/native_client/src/native_client/+/5152c12c4... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/native_client/src/native_client/+/5152c12c4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
