Re: [PATCH v5 00/18] Rework READ_ONCE() to improve codegen

[Martin LiÅka]

On 6/8/20 9:56 PM, Marco Elver wrote:
> On Mon, 8 Jun 2020 at 19:32, Martin LiÅka <mliska@xxxxxxx> wrote:
> > On 6/3/20 9:23 PM, Marco Elver wrote:
> > > On Wed, 03 Jun 2020, Borislav Petkov wrote:
> > >
> > > > On Thu, May 14, 2020 at 12:05:38PM +0100, Will Deacon wrote:
> > > > > Talking off-list, Clang >= 7 is pretty reasonable wrt inlining decisions
> > > > > and the behaviour for __always_inline is:
> > > > >
> > > > >     * An __always_inline function inlined into a __no_sanitize function is
> > > > >       not instrumented
> > > > >     * An __always_inline function inlined into an instrumented function is
> > > > >       instrumented
> > > > >     * You can't mark a function as both __always_inline __no_sanitize, because
> > > > >       __no_sanitize functions are never inlined
> > > > >
> > > > > GCC, on the other hand, may still inline __no_sanitize functions and then
> > > > > subsequently instrument them.
> > > >
> > > > Yeah, about that: I've been looking for a way to trigger this so that
> > > > I can show preprocessed source to gcc people. So do you guys have a
> > > > .config or somesuch I can try?
> > >
> > > For example take this:
> > >
> > >        int x;
> > >
> > >        static inline __attribute__((no_sanitize_thread)) void do_not_sanitize(void) {
> > >          x++;
> > >        }
> > >
> > >        void sanitize_this(void) {
> > >          do_not_sanitize();
> > >        }
> > >
> > > Then
> > >
> > >        gcc-10 -O3 -fsanitize=thread -o example.o -c example.c
> > >        objdump -D example.o
> >
> > Hello.
> >
> > Thank you for the example. It seems to me that Clang does not inline a no_sanitize_* function
> > into one which is instrumented. Is it a documented behavior ([1] doesn't mention that)?
> > If so, we can do the same in GCC.
>
> It is not explicitly mentioned in [1]. But the contract of
> "no_sanitize" is "that a particular instrumentation or set of
> instrumentations should not be applied". That contract is broken if a
> function is instrumented, however that may happen. It sadly does
> happen with GCC when a function is inlined. Presumably because the
> sanitizer passes for TSAN/ASAN/MSAN run after the optimizer -- this
> definitely can't change. Also because it currently gives us the
> property that __always_inline functions are instrumented according to
> the function they are inlined into (a property we want).
>
> The easy fix to no_sanitize seems to be to do what Clang does, and
> never inline no_sanitize functions (with or without "inline"
> attribute).  always_inline functions should remain unchanged
> (specifying no_sanitize on an always_inline function is an error).

Hello.

Works for me and I've just sent patch for that:
https://gcc.gnu.org/pipermail/gcc-patches/2020-June/547618.html

> Note this applies to all sanitizers (TSAN/ASAN/MSAN) and their
> no_sanitize attribute that GCC has.

Sure.

> The list of requirements were also summarized in more detail here:
> https://lore.kernel.org/lkml/CANpmjNMTsY_8241bS7=XAfqvZHFLrVEkv_uM4aDUWE_kh3Rvbw@xxxxxxxxxxxxxx/
>
> Hope that makes sense. (I also need to send a v2 for param
> tsan-distinguish-volatile, but haven't gotten around to it yet --
> hopefully soon.

The patch is approved now.

 And then we also need a param
> tsan-instrument-func-entry-exit, which LLVM has for TSAN. One step at
> a time though.)

Yes, please send a patch for it.

Martin

> Thanks,
> -- Marco
>
>
> > Thanks,
> > Martin
> >
> > [1] https://clang.llvm.org/docs/AttributeReference.html#no-sanitize
> >
> > > will show that do_not_sanitize() was inlined into sanitize_this() and is
> > > instrumented. (With Clang this doesn't happen.)
> > >
> > > Hope this is enough.
> > >
> > > Thanks,
> > > -- Marco