From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 89408 invoked by alias); 2 Oct 2017 20:12:30 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 88165 invoked by uid 89); 2 Oct 2017 20:12:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-7.7 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_2,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy=tough, architectural, voice, specifics X-HELO: mail-pf0-f180.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=Y8l/HWW+GtE0LxpHlCDUwfcHAJzW6ss/dsYJWB55jv8=; b=pJ1SLixj0QWvRWr6agbq4OcDy6Vqw+3yghYG0SZ5DuRccwcBM37Px+MXNn+89GdW5X Jtqs0SNUopqJrXseVcMZ+aVTrHxGhlRV2dFjHZ5tLxlTE4xF2zymZ69OhpZUf6hIjdFc gVhTFmEu65Me/BB3lyKL2Gg2TcuqE+J6QJ4+YaO3SgQYyB9R09kG9z6SDty5n8D/FbsU VfR2ZbUtMkGobViSPNGU2J5iv+kyPvuHsY8oAIAZfaew2SNOitdA8zdL4uLzF2xgqFg4 Vg7GwZGO0oUeWtwv8I9aIBpOLiEktbRZMyqNCnQvzyxZSf7Z/Q+kjT1gCcyLaYKOAOMG he6w== X-Gm-Message-State: AHPjjUiJe9AsO9KXeGXyCQ7iV66AWmzH7U3RGKUMEWh4D1KEyGO8uvKi /JQw2obzXRd6dEUg/OMJb5CDlyybUYTcKBg2LKK2kg== X-Google-Smtp-Source: AOwi7QDYFgE6d12oONtw8wD1PCcUu/rXAgWx5BOWNM1uBKt5q0Gq65BBKVhAavnWNQgqxm0f9DmDgjyeoje8kkerE5s= X-Received: by 10.159.204.139 with SMTP id t11mr15189695plo.359.1506975140960; Mon, 02 Oct 2017 13:12:20 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: From: George Burgess IV Date: Mon, 02 Oct 2017 20:12:00 -0000 Message-ID: Subject: Re: [RFC][PATCH] Refactoring FORTIFY To: libc-alpha@sourceware.org Content-Type: text/plain; charset="UTF-8" X-SW-Source: 2017-10/txt/msg00078.txt.bz2 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 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.