From: George Burgess IV <george.burgess.iv@gmail.com>
To: libc-alpha@sourceware.org
Subject: Re: [RFC][PATCH] Refactoring FORTIFY
Date: Mon, 02 Oct 2017 20:12:00 -0000 [thread overview]
Message-ID: <CAKh6zBFKxhmYbH7iTQvDVe3GpLnzwsMs48hHmw1OW_qnN7bTOg@mail.gmail.com> (raw)
In-Reply-To: <CAKh6zBFAGomCFc+Y9=qpH-2N6AonKUja1R+cWy=p_=T=rs29cQ@mail.gmail.com>
Ping. :)
(If I don't get a response after a few more pings, I'll go ahead
and split this up/try to get it reviewed.)
On Sun, Sep 10, 2017 at 11:26 PM, George Burgess IV
<george.burgess.iv@gmail.com> wrote:
> Hello,
>
> Attached is a patch that aims to substantially improve FORTIFY's
> usefulness with clang, and make defining FORTIFY'ed functions require
> less ceremony.
>
> I'm not looking for a thorough review at this time, nor do I hope to
> land this patch in one piece. The goal of this thread is to ensure
> that everyone's more or less happy with where I'd like to take glibc's
> FORTIFY implementation.
>
> Please note that this change is intended to be a functional nop for
> all compilers that aren't clang >= 5.0, which was just released last
> Thursday.
>
> Diving in: as said, this patch removes a lot of duplication from
> FORTIFY in the common case, and makes FORTIFY far more useful for
> those who use glibc with clang. Namely, with this patch, clang becomes
> capable of emitting compile-time diagnostics on par (*) with GCC's,
> and clang's ability to perform run-time checks is substantially
> improved over what we have today.
>
> It essentially does this by wrapping up the majority of the
> compiler-specific incantations (declaring __foo_chk_warn,
> conditionally calling it, ...) behind a macro, and uses that to stamp
> out FORTIFY's compile-time diagnostic bits. While this approach is the
> cleanest I've been able to come up with, it has potential downsides:
>
> - Compile-time diagnostics with GCC are somewhat different than what
> they are today. To show this, I've attached tst-chk2-output.diff,
> which is a diff of the diagnostics produced by running GCC 7.1 on
> debug/tst-chk2.c. I don't find the difference to be substantial, but
> it does exist.
> - In very rare cases, the code generated by GCC will be a bit worse
> (e.g. slower+larger) with this patch. I know this may be a tough sell,
> but please hear me out. :)
>
> With this patch, we sometimes emit diagnostics by emitting code like:
> if (should_emit_compile_time_warning) {
> volatile char c = 0;
> if (__glibc_unlikely (c))
> function_with_warnattr ();
> }
>
> Where `should_emit_compile_time_warning` always folds to a constant
> during compilation. So, 0 diagnostics should mean 0 change in code
> quality.
>
> I don't believe this is a deal-breaker, since:
> - if you're using FORTIFY, you presumably care enough to fix
> FORTIFY-related warnings, which makes this regression nonexistent for
> you,
> - if you're using FORTIFY, you know there will be a (small, but
> present) performance penalty,
> - directly after each __glibc_unlikely(c) branch is call to a FORTIFY
> function with known broken input, which should abort the program
> anyway, and
> - this doesn't apply to *all* diagnostics (e.g. bad calls to open()
> don't have this penalty); just ones where we can unify clang's and
> GCC's diagnostic emission bits.
>
> In any case, please see binary-output.diff for an idea of the
> difference this makes on code compiled with GCC 7.1. The function
> being compiled was a call to wmemmove with an undersized buffer. With
> a sufficiently large buffer, both today's FORTIFY implementation and
> the proposed one produce identical bodies for the function in
> question.
>
> Other than that, I know of no regressions that this patch causes with GCC.
>
> For clang, in very rare cases (read: I've seen ~10 instances of this
> testing similar implementations across Android, ChromeOS, and another
> very large code base), it can also break existing code. For specifics
> on that, and an overview on how clang's FORTIFY implementation works,
> please see
> https://docs.google.com/document/d/1DFfZDICTbL7RqS74wJVIJ-YnjQOj1SaoqfhbgddFYSM/edit?usp=sharing
> The "How does clang handle it?" section covers the primary attributes
> this patch uses, and the "Incompatibilities between clang and GCC
> FORTIFY" section covers places where this patch might break existing
> clang users.
>
> Though I expect clang breakages to be very rare and trivial to fix,
> this patch introduces a _CLANG_FORTIFY_DISABLE macro, which can be
> used to turn this entire patch into a nop for clang >= 5.0.
>
> -----
>
> I apologize if this is a lot to dump into one email. As said, if we
> decide this is an acceptable direction to head in, I hope to land this
> patch in many easily reviewable parts later on.
>
> If you have any questions, comments, concerns, etc. please don't
> hesitate to voice them.
>
> Thank you very much for your time, :)
> George
>
> (*) - This means that clang can catch almost as many *types* of bugs
> as GCC at compile-time. Due to architectural constraints, clang can
> only perform these checks prior to optimizing code.
> So, clang is still unable to statically diagnose bugs that require it
> to do more than simple constant folding. __builtin_object_size,
> however, isn't required to be folded until after we optimize code, so
> many dynamic checks can still take advantage of optimizations.
next prev parent reply other threads:[~2017-10-02 20:12 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-11 6:27 George Burgess IV
2017-10-02 20:12 ` George Burgess IV [this message]
2017-10-03 19:34 ` Adhemerval Zanella
2017-10-03 20:54 ` George Burgess IV
2017-10-05 13:43 ` Adhemerval Zanella
2017-10-05 15:48 ` Joseph Myers
2017-10-05 20:06 ` Adhemerval Zanella
2017-11-06 19:12 ` George Burgess IV
2017-11-20 20:03 ` Adhemerval Zanella
2017-12-05 4:53 ` George Burgess IV
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKh6zBFKxhmYbH7iTQvDVe3GpLnzwsMs48hHmw1OW_qnN7bTOg@mail.gmail.com \
--to=george.burgess.iv@gmail.com \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).