mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 52695: Harden libprocess
Date Fri, 11 May 2018 21:34:05 GMT


> On Nov. 2, 2016, 9:32 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/Makefile.am
> > Lines 30 (patched)
> > <https://reviews.apache.org/r/52695/diff/4/?file=1551006#file1551006line30>
> >
> >     I am not a big fan of unconditionally omitting frame pointers as this gives
the optimizer one less register to work with. Unfortunately one cannot easily tell the actual
impact of this from the info here. Is this strictly needed here or just nice to have?
> 
> James Peach wrote:
>     The performance benefit of omitting frame pointers is likely to be marginal on x64_64,
if it is a win at all. The rationale for adding this is that it makes stack walking reliable
in all cases, so debugability is improved and you can get reasonable results when uting `perf`.
Since most users will build with default options I suggested to Aaron that we should make
it the default.
> 
> Benjamin Bannier wrote:
>     Thanks James, that makes sense.
>     
>     Since this seems all related to debugability what about enabling it _only for builds
with `--enable-debug`_ (e.g., perf results already now also don't necessarily give full info
w/o debug symbols)? Tangentially related, tcmalloc can fail in debug builds with omitted frame
pointers, so disabling `omit-frame-pointer` in debug builds might safe us from some future
headaches, https://bugs.chromium.org/p/chromium/issues/detail?id=636489.
>     
>     `stack-protector-strong` can significantly increase the binary size, and we should
either only enable it for e.g., debug builds, or give users a `configure` knob to disable
it.
>     
>     For using `FORTIFY_SOURCE` I think we also need be a little more careful. Support
for it is somewhat broken in clang (https://llvm.org/bugs/show_bug.cgi?id=16821), it only
has useful effects in builds with some level of optimization, and can e.g., mess up reports
from sanitizers injected by users. I can see good uses for a `configure` flag to disable this
compiler flag, but I am not sure what the default should be.
> 
> Aaron Wood wrote:
>     Going to drop this since we've all agreed on Slack to have the frame pointer modification
done in a separate patch.

Looks like there wasn't a patch for `-fno-omit-frame-pointer`?

Filed: https://issues.apache.org/jira/browse/MESOS-8908


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52695/#review154527
-----------------------------------------------------------


On Nov. 30, 2016, 8:52 p.m., Aaron Wood wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52695/
> -----------------------------------------------------------
> 
> (Updated Nov. 30, 2016, 8:52 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6229
>     https://issues.apache.org/jira/browse/MESOS-6229
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add hardened flags for libprocess.
> Take compile flag macro at 391cb680171d3889965b1ead43d3a326c913bc25.
> The macro at 1a869696e4129279f7b99c3f9052717354b79a86 requires autoconf 2.64 which breaks
on CentOS 6.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 9d496b8 
>   3rdparty/libprocess/configure.ac e65e5ca 
>   3rdparty/libprocess/m4/ax_check_compile_flag.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/52695/diff/9/
> 
> 
> Testing
> -------
> 
> Compared the benchmarks with and without the flags being used. Also did a comparsion
with the flags being used with and without optimizations and without the flags being used
with and without optimizations. Overall the performance hit was very small with a 3-8% overhead
(optimizations brings this down slightly). Most benchmarks were about 5% (or less) slower.
> 
> 
> File Attachments
> ----------------
> 
> --enable-optimized with hardening applied
>   https://reviews.apache.org/media/uploaded/files/2016/11/02/875c9e6e-c73b-4e3c-8265-0f7c6dc00351__hardened-optimized.txt
> Hardening applied but no --enable-optimized
>   https://reviews.apache.org/media/uploaded/files/2016/11/02/932d28a7-2d31-471a-b438-647841a6853c__hardened-unoptimized.txt
> --enable-optimized with no hardening applied
>   https://reviews.apache.org/media/uploaded/files/2016/11/02/896944ea-9b31-4d62-b1b9-97fb4700a882__optimized.txt
> No hardening applied and no --enable-optimized
>   https://reviews.apache.org/media/uploaded/files/2016/11/02/b32667ce-3e3b-4d2b-b4f8-4c2404a0fc1c__unoptimized.txt
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message