Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Oct 10, 2018 at 7:48 PM, Masahiro Yamada wrote: > On Wed, Oct 10, 2018 at 11:51 PM Kees Cook wrote: >> >> On Wed, Oct 10, 2018 at 12:03 AM, Miguel Ojeda >> wrote: >> > On Wed, Oct 10, 2018 at 8:12 AM Joel Stanley wrote: >> >> >> >> On Thu, 27 Sep 2018 at 05:07, Kees Cook wrote: >> >> > >> >> > Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you >> >> > take this? >> >> >> >> clang built -next is blowing up now that Kees' -Wvla patch has been >> >> included. This patch fixes it. >> >> >> >> Kees, perhaps it should go in your tree along side of the -Wvla patch >> >> if no one else wants to take it? >> >> >> > >> > I can take it along in the compiler attributes tree, since that >> > touches the compiler*.h stuff. Although that would make it >> > not-only-attributes, i.e. slightly lying :-) >> >> Oh, I had assumed Masahiro was going to carry it. > > No, I am not. > > Putting all sort of things into kbuild basket > is painful for me. Okay, I'll take it for the VLA series. -Kees -- Kees Cook Pixel Security
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Oct 10, 2018 at 7:48 PM, Masahiro Yamada wrote: > On Wed, Oct 10, 2018 at 11:51 PM Kees Cook wrote: >> >> On Wed, Oct 10, 2018 at 12:03 AM, Miguel Ojeda >> wrote: >> > On Wed, Oct 10, 2018 at 8:12 AM Joel Stanley wrote: >> >> >> >> On Thu, 27 Sep 2018 at 05:07, Kees Cook wrote: >> >> > >> >> > Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you >> >> > take this? >> >> >> >> clang built -next is blowing up now that Kees' -Wvla patch has been >> >> included. This patch fixes it. >> >> >> >> Kees, perhaps it should go in your tree along side of the -Wvla patch >> >> if no one else wants to take it? >> >> >> > >> > I can take it along in the compiler attributes tree, since that >> > touches the compiler*.h stuff. Although that would make it >> > not-only-attributes, i.e. slightly lying :-) >> >> Oh, I had assumed Masahiro was going to carry it. > > No, I am not. > > Putting all sort of things into kbuild basket > is painful for me. Okay, I'll take it for the VLA series. -Kees -- Kees Cook Pixel Security
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Oct 10, 2018 at 11:51 PM Kees Cook wrote: > > On Wed, Oct 10, 2018 at 12:03 AM, Miguel Ojeda > wrote: > > On Wed, Oct 10, 2018 at 8:12 AM Joel Stanley wrote: > >> > >> On Thu, 27 Sep 2018 at 05:07, Kees Cook wrote: > >> > > >> > Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you > >> > take this? > >> > >> clang built -next is blowing up now that Kees' -Wvla patch has been > >> included. This patch fixes it. > >> > >> Kees, perhaps it should go in your tree along side of the -Wvla patch > >> if no one else wants to take it? > >> > > > > I can take it along in the compiler attributes tree, since that > > touches the compiler*.h stuff. Although that would make it > > not-only-attributes, i.e. slightly lying :-) > > Oh, I had assumed Masahiro was going to carry it. No, I am not. Putting all sort of things into kbuild basket is painful for me. > If that's not true, > sure I'll pick it up as part of my VLA "series". -- Best Regards Masahiro Yamada
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Oct 10, 2018 at 11:51 PM Kees Cook wrote: > > On Wed, Oct 10, 2018 at 12:03 AM, Miguel Ojeda > wrote: > > On Wed, Oct 10, 2018 at 8:12 AM Joel Stanley wrote: > >> > >> On Thu, 27 Sep 2018 at 05:07, Kees Cook wrote: > >> > > >> > Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you > >> > take this? > >> > >> clang built -next is blowing up now that Kees' -Wvla patch has been > >> included. This patch fixes it. > >> > >> Kees, perhaps it should go in your tree along side of the -Wvla patch > >> if no one else wants to take it? > >> > > > > I can take it along in the compiler attributes tree, since that > > touches the compiler*.h stuff. Although that would make it > > not-only-attributes, i.e. slightly lying :-) > > Oh, I had assumed Masahiro was going to carry it. No, I am not. Putting all sort of things into kbuild basket is painful for me. > If that's not true, > sure I'll pick it up as part of my VLA "series". -- Best Regards Masahiro Yamada
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Oct 10, 2018 at 12:03 AM, Miguel Ojeda wrote: > On Wed, Oct 10, 2018 at 8:12 AM Joel Stanley wrote: >> >> On Thu, 27 Sep 2018 at 05:07, Kees Cook wrote: >> > >> > Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you take >> > this? >> >> clang built -next is blowing up now that Kees' -Wvla patch has been >> included. This patch fixes it. >> >> Kees, perhaps it should go in your tree along side of the -Wvla patch >> if no one else wants to take it? >> > > I can take it along in the compiler attributes tree, since that > touches the compiler*.h stuff. Although that would make it > not-only-attributes, i.e. slightly lying :-) Oh, I had assumed Masahiro was going to carry it. If that's not true, sure I'll pick it up as part of my VLA "series". -Kees -- Kees Cook Pixel Security
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Oct 10, 2018 at 12:03 AM, Miguel Ojeda wrote: > On Wed, Oct 10, 2018 at 8:12 AM Joel Stanley wrote: >> >> On Thu, 27 Sep 2018 at 05:07, Kees Cook wrote: >> > >> > Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you take >> > this? >> >> clang built -next is blowing up now that Kees' -Wvla patch has been >> included. This patch fixes it. >> >> Kees, perhaps it should go in your tree along side of the -Wvla patch >> if no one else wants to take it? >> > > I can take it along in the compiler attributes tree, since that > touches the compiler*.h stuff. Although that would make it > not-only-attributes, i.e. slightly lying :-) Oh, I had assumed Masahiro was going to carry it. If that's not true, sure I'll pick it up as part of my VLA "series". -Kees -- Kees Cook Pixel Security
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Oct 10, 2018 at 8:12 AM Joel Stanley wrote: > > On Thu, 27 Sep 2018 at 05:07, Kees Cook wrote: > > > > Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you take > > this? > > clang built -next is blowing up now that Kees' -Wvla patch has been > included. This patch fixes it. > > Kees, perhaps it should go in your tree along side of the -Wvla patch > if no one else wants to take it? > I can take it along in the compiler attributes tree, since that touches the compiler*.h stuff. Although that would make it not-only-attributes, i.e. slightly lying :-) Cheers, Miguel
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Oct 10, 2018 at 8:12 AM Joel Stanley wrote: > > On Thu, 27 Sep 2018 at 05:07, Kees Cook wrote: > > > > Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you take > > this? > > clang built -next is blowing up now that Kees' -Wvla patch has been > included. This patch fixes it. > > Kees, perhaps it should go in your tree along side of the -Wvla patch > if no one else wants to take it? > I can take it along in the compiler attributes tree, since that touches the compiler*.h stuff. Although that would make it not-only-attributes, i.e. slightly lying :-) Cheers, Miguel
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Thu, 27 Sep 2018 at 05:07, Kees Cook wrote: > > On Wed, Sep 26, 2018 at 12:29 PM, Nick Desaulniers > wrote: > > On Wed, Sep 26, 2018 at 12:03 PM Greg KH wrote: > >> > >> On Wed, Sep 26, 2018 at 11:45:19AM -0700, Kees Cook wrote: > >> > On Wed, Sep 26, 2018 at 11:42 AM, Greg KH > >> > wrote: > >> > > I'm not digging up a compiler.h patch from a web site and adding it to > >> > > the tree this late in the release cycle. Especially given that it > >> > > hasn't had any testing anywhere... > >> > > >> > Good point about it not living in -next. > >> > > >> > Who should be carrying these sorts of patches? In the past it's been > >> > Andrew or Masahiro, yes? For linux-next, maybe it can go via -mm? > >> > >> Either is fine with me, as long as it isn't one of my trees :) > >> > >> thanks, > >> > >> greg k-h > > > > Besides, I think we want the v2: https://lkml.org/lkml/2018/8/25/103 > > Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you take > this? clang built -next is blowing up now that Kees' -Wvla patch has been included. This patch fixes it. Kees, perhaps it should go in your tree along side of the -Wvla patch if no one else wants to take it? Cheers, Joel
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Thu, 27 Sep 2018 at 05:07, Kees Cook wrote: > > On Wed, Sep 26, 2018 at 12:29 PM, Nick Desaulniers > wrote: > > On Wed, Sep 26, 2018 at 12:03 PM Greg KH wrote: > >> > >> On Wed, Sep 26, 2018 at 11:45:19AM -0700, Kees Cook wrote: > >> > On Wed, Sep 26, 2018 at 11:42 AM, Greg KH > >> > wrote: > >> > > I'm not digging up a compiler.h patch from a web site and adding it to > >> > > the tree this late in the release cycle. Especially given that it > >> > > hasn't had any testing anywhere... > >> > > >> > Good point about it not living in -next. > >> > > >> > Who should be carrying these sorts of patches? In the past it's been > >> > Andrew or Masahiro, yes? For linux-next, maybe it can go via -mm? > >> > >> Either is fine with me, as long as it isn't one of my trees :) > >> > >> thanks, > >> > >> greg k-h > > > > Besides, I think we want the v2: https://lkml.org/lkml/2018/8/25/103 > > Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you take > this? clang built -next is blowing up now that Kees' -Wvla patch has been included. This patch fixes it. Kees, perhaps it should go in your tree along side of the -Wvla patch if no one else wants to take it? Cheers, Joel
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Sep 26, 2018 at 12:29 PM, Nick Desaulniers wrote: > On Wed, Sep 26, 2018 at 12:03 PM Greg KH wrote: >> >> On Wed, Sep 26, 2018 at 11:45:19AM -0700, Kees Cook wrote: >> > On Wed, Sep 26, 2018 at 11:42 AM, Greg KH >> > wrote: >> > > I'm not digging up a compiler.h patch from a web site and adding it to >> > > the tree this late in the release cycle. Especially given that it >> > > hasn't had any testing anywhere... >> > >> > Good point about it not living in -next. >> > >> > Who should be carrying these sorts of patches? In the past it's been >> > Andrew or Masahiro, yes? For linux-next, maybe it can go via -mm? >> >> Either is fine with me, as long as it isn't one of my trees :) >> >> thanks, >> >> greg k-h > > Besides, I think we want the v2: https://lkml.org/lkml/2018/8/25/103 Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you take this? -Kees -- Kees Cook Pixel Security
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Sep 26, 2018 at 12:29 PM, Nick Desaulniers wrote: > On Wed, Sep 26, 2018 at 12:03 PM Greg KH wrote: >> >> On Wed, Sep 26, 2018 at 11:45:19AM -0700, Kees Cook wrote: >> > On Wed, Sep 26, 2018 at 11:42 AM, Greg KH >> > wrote: >> > > I'm not digging up a compiler.h patch from a web site and adding it to >> > > the tree this late in the release cycle. Especially given that it >> > > hasn't had any testing anywhere... >> > >> > Good point about it not living in -next. >> > >> > Who should be carrying these sorts of patches? In the past it's been >> > Andrew or Masahiro, yes? For linux-next, maybe it can go via -mm? >> >> Either is fine with me, as long as it isn't one of my trees :) >> >> thanks, >> >> greg k-h > > Besides, I think we want the v2: https://lkml.org/lkml/2018/8/25/103 Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you take this? -Kees -- Kees Cook Pixel Security
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Sep 26, 2018 at 12:03 PM Greg KH wrote: > > On Wed, Sep 26, 2018 at 11:45:19AM -0700, Kees Cook wrote: > > On Wed, Sep 26, 2018 at 11:42 AM, Greg KH > > wrote: > > > I'm not digging up a compiler.h patch from a web site and adding it to > > > the tree this late in the release cycle. Especially given that it > > > hasn't had any testing anywhere... > > > > Good point about it not living in -next. > > > > Who should be carrying these sorts of patches? In the past it's been > > Andrew or Masahiro, yes? For linux-next, maybe it can go via -mm? > > Either is fine with me, as long as it isn't one of my trees :) > > thanks, > > greg k-h Besides, I think we want the v2: https://lkml.org/lkml/2018/8/25/103 -- Thanks, ~Nick Desaulniers
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Sep 26, 2018 at 12:03 PM Greg KH wrote: > > On Wed, Sep 26, 2018 at 11:45:19AM -0700, Kees Cook wrote: > > On Wed, Sep 26, 2018 at 11:42 AM, Greg KH > > wrote: > > > I'm not digging up a compiler.h patch from a web site and adding it to > > > the tree this late in the release cycle. Especially given that it > > > hasn't had any testing anywhere... > > > > Good point about it not living in -next. > > > > Who should be carrying these sorts of patches? In the past it's been > > Andrew or Masahiro, yes? For linux-next, maybe it can go via -mm? > > Either is fine with me, as long as it isn't one of my trees :) > > thanks, > > greg k-h Besides, I think we want the v2: https://lkml.org/lkml/2018/8/25/103 -- Thanks, ~Nick Desaulniers
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Sep 26, 2018 at 11:45:19AM -0700, Kees Cook wrote: > On Wed, Sep 26, 2018 at 11:42 AM, Greg KH wrote: > > I'm not digging up a compiler.h patch from a web site and adding it to > > the tree this late in the release cycle. Especially given that it > > hasn't had any testing anywhere... > > Good point about it not living in -next. > > Who should be carrying these sorts of patches? In the past it's been > Andrew or Masahiro, yes? For linux-next, maybe it can go via -mm? Either is fine with me, as long as it isn't one of my trees :) thanks, greg k-h
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Sep 26, 2018 at 11:45:19AM -0700, Kees Cook wrote: > On Wed, Sep 26, 2018 at 11:42 AM, Greg KH wrote: > > I'm not digging up a compiler.h patch from a web site and adding it to > > the tree this late in the release cycle. Especially given that it > > hasn't had any testing anywhere... > > Good point about it not living in -next. > > Who should be carrying these sorts of patches? In the past it's been > Andrew or Masahiro, yes? For linux-next, maybe it can go via -mm? Either is fine with me, as long as it isn't one of my trees :) thanks, greg k-h
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Sep 26, 2018 at 11:42 AM, Greg KH wrote: > I'm not digging up a compiler.h patch from a web site and adding it to > the tree this late in the release cycle. Especially given that it > hasn't had any testing anywhere... Good point about it not living in -next. Who should be carrying these sorts of patches? In the past it's been Andrew or Masahiro, yes? For linux-next, maybe it can go via -mm? -Kees -- Kees Cook Pixel Security
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Sep 26, 2018 at 11:42 AM, Greg KH wrote: > I'm not digging up a compiler.h patch from a web site and adding it to > the tree this late in the release cycle. Especially given that it > hasn't had any testing anywhere... Good point about it not living in -next. Who should be carrying these sorts of patches? In the past it's been Andrew or Masahiro, yes? For linux-next, maybe it can go via -mm? -Kees -- Kees Cook Pixel Security
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Sep 26, 2018 at 11:26:46AM -0700, Kees Cook wrote:
> On Wed, Sep 26, 2018 at 11:03 AM, Nick Desaulniers
> wrote:
> > On Wed, Sep 26, 2018 at 11:00 AM Matthias Kaehlcke
> > wrote:
> >>
> >> On Fri, Aug 31, 2018 at 09:46:02AM -0700, Nick Desaulniers wrote:
> >> > On Tue, Aug 28, 2018 at 4:00 PM Nick Desaulniers
> >> > wrote:
> >> > >
> >> > > On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos
> >> > > wrote:
> >> > > >
> >> > > > Hello Nick,
> >> > > >
> >> > > > On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> >> > > > >>> Let's give up __compiletime_assert_fallback(). This commit does
> >> > > > >>> not
> >> > > > >>> change the current behavior since it just rips off the useless
> >> > > > >>> code.
> >> > > > >> Clang is not the only target audience of
> >> > > > >> __compiletime_assert_fallback(). Instead of ripping out
> >> > > > >> something that
> >> > > > >> may benefit builds with gcc 4.2 and earlier, why not override its
> >> > > > > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> >> > > > > version to 4.6") that gcc < 4.6 is irrelevant.
> >> > > >
> >> > > > Ah, I guess I'm not keeping up, that's wonderful news! Considering
> >> > > > that
> >> > > > I guess I would be OK with its removal, but I still think it would be
> >> > > > better if a similar mechanism to break the Clang build could be
> >> > > > found.
> >> > >
> >> > > I'm consulting with our best language lawyers to see what combinations
> >> > > of _Static_assert and __builtin_constant_p would do the trick.
> >> >
> >> > Linus,
> >> > Can this patch be merged in the meantime?
> >>
> >> friendly ping :)
> >>
> >> With c5c2b11894f4 ("drm/i915: Warn against variable length arrays")
> >> clang raises plenty of vla warnings about
> >> __compiletime_error_fallback() in the i915 driver. Would be great to
> >> get rid of those without having to revert that commit.
> >
> > I've been meaning to follow up on this, thanks Matthias. I too would
> > really like this patch.
>
> Adding Greg to the thread. Between Masahiro's detailed commit log and
> the Clang-familiar reviewers, I think this should land for 4.19 (as
> part of the other Clang-sanity patches that are already in 4.19). This
> has no impact on gcc now that we're requiring 4.6+.
>
> https://lore.kernel.org/patchwork/patch/977668/
I'm not digging up a compiler.h patch from a web site and adding it to
the tree this late in the release cycle. Especially given that it
hasn't had any testing anywhere...
nice try though :)
greg k-h
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Sep 26, 2018 at 11:26:46AM -0700, Kees Cook wrote:
> On Wed, Sep 26, 2018 at 11:03 AM, Nick Desaulniers
> wrote:
> > On Wed, Sep 26, 2018 at 11:00 AM Matthias Kaehlcke
> > wrote:
> >>
> >> On Fri, Aug 31, 2018 at 09:46:02AM -0700, Nick Desaulniers wrote:
> >> > On Tue, Aug 28, 2018 at 4:00 PM Nick Desaulniers
> >> > wrote:
> >> > >
> >> > > On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos
> >> > > wrote:
> >> > > >
> >> > > > Hello Nick,
> >> > > >
> >> > > > On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> >> > > > >>> Let's give up __compiletime_assert_fallback(). This commit does
> >> > > > >>> not
> >> > > > >>> change the current behavior since it just rips off the useless
> >> > > > >>> code.
> >> > > > >> Clang is not the only target audience of
> >> > > > >> __compiletime_assert_fallback(). Instead of ripping out
> >> > > > >> something that
> >> > > > >> may benefit builds with gcc 4.2 and earlier, why not override its
> >> > > > > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> >> > > > > version to 4.6") that gcc < 4.6 is irrelevant.
> >> > > >
> >> > > > Ah, I guess I'm not keeping up, that's wonderful news! Considering
> >> > > > that
> >> > > > I guess I would be OK with its removal, but I still think it would be
> >> > > > better if a similar mechanism to break the Clang build could be
> >> > > > found.
> >> > >
> >> > > I'm consulting with our best language lawyers to see what combinations
> >> > > of _Static_assert and __builtin_constant_p would do the trick.
> >> >
> >> > Linus,
> >> > Can this patch be merged in the meantime?
> >>
> >> friendly ping :)
> >>
> >> With c5c2b11894f4 ("drm/i915: Warn against variable length arrays")
> >> clang raises plenty of vla warnings about
> >> __compiletime_error_fallback() in the i915 driver. Would be great to
> >> get rid of those without having to revert that commit.
> >
> > I've been meaning to follow up on this, thanks Matthias. I too would
> > really like this patch.
>
> Adding Greg to the thread. Between Masahiro's detailed commit log and
> the Clang-familiar reviewers, I think this should land for 4.19 (as
> part of the other Clang-sanity patches that are already in 4.19). This
> has no impact on gcc now that we're requiring 4.6+.
>
> https://lore.kernel.org/patchwork/patch/977668/
I'm not digging up a compiler.h patch from a web site and adding it to
the tree this late in the release cycle. Especially given that it
hasn't had any testing anywhere...
nice try though :)
greg k-h
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Sep 26, 2018 at 11:03 AM, Nick Desaulniers
wrote:
> On Wed, Sep 26, 2018 at 11:00 AM Matthias Kaehlcke wrote:
>>
>> On Fri, Aug 31, 2018 at 09:46:02AM -0700, Nick Desaulniers wrote:
>> > On Tue, Aug 28, 2018 at 4:00 PM Nick Desaulniers
>> > wrote:
>> > >
>> > > On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos
>> > > wrote:
>> > > >
>> > > > Hello Nick,
>> > > >
>> > > > On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
>> > > > >>> Let's give up __compiletime_assert_fallback(). This commit does
>> > > > >>> not
>> > > > >>> change the current behavior since it just rips off the useless
>> > > > >>> code.
>> > > > >> Clang is not the only target audience of
>> > > > >> __compiletime_assert_fallback(). Instead of ripping out something
>> > > > >> that
>> > > > >> may benefit builds with gcc 4.2 and earlier, why not override its
>> > > > > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
>> > > > > version to 4.6") that gcc < 4.6 is irrelevant.
>> > > >
>> > > > Ah, I guess I'm not keeping up, that's wonderful news! Considering
>> > > > that
>> > > > I guess I would be OK with its removal, but I still think it would be
>> > > > better if a similar mechanism to break the Clang build could be found.
>> > >
>> > > I'm consulting with our best language lawyers to see what combinations
>> > > of _Static_assert and __builtin_constant_p would do the trick.
>> >
>> > Linus,
>> > Can this patch be merged in the meantime?
>>
>> friendly ping :)
>>
>> With c5c2b11894f4 ("drm/i915: Warn against variable length arrays")
>> clang raises plenty of vla warnings about
>> __compiletime_error_fallback() in the i915 driver. Would be great to
>> get rid of those without having to revert that commit.
>
> I've been meaning to follow up on this, thanks Matthias. I too would
> really like this patch.
Adding Greg to the thread. Between Masahiro's detailed commit log and
the Clang-familiar reviewers, I think this should land for 4.19 (as
part of the other Clang-sanity patches that are already in 4.19). This
has no impact on gcc now that we're requiring 4.6+.
https://lore.kernel.org/patchwork/patch/977668/
-Kees
--
Kees Cook
Pixel Security
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Sep 26, 2018 at 11:03 AM, Nick Desaulniers
wrote:
> On Wed, Sep 26, 2018 at 11:00 AM Matthias Kaehlcke wrote:
>>
>> On Fri, Aug 31, 2018 at 09:46:02AM -0700, Nick Desaulniers wrote:
>> > On Tue, Aug 28, 2018 at 4:00 PM Nick Desaulniers
>> > wrote:
>> > >
>> > > On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos
>> > > wrote:
>> > > >
>> > > > Hello Nick,
>> > > >
>> > > > On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
>> > > > >>> Let's give up __compiletime_assert_fallback(). This commit does
>> > > > >>> not
>> > > > >>> change the current behavior since it just rips off the useless
>> > > > >>> code.
>> > > > >> Clang is not the only target audience of
>> > > > >> __compiletime_assert_fallback(). Instead of ripping out something
>> > > > >> that
>> > > > >> may benefit builds with gcc 4.2 and earlier, why not override its
>> > > > > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
>> > > > > version to 4.6") that gcc < 4.6 is irrelevant.
>> > > >
>> > > > Ah, I guess I'm not keeping up, that's wonderful news! Considering
>> > > > that
>> > > > I guess I would be OK with its removal, but I still think it would be
>> > > > better if a similar mechanism to break the Clang build could be found.
>> > >
>> > > I'm consulting with our best language lawyers to see what combinations
>> > > of _Static_assert and __builtin_constant_p would do the trick.
>> >
>> > Linus,
>> > Can this patch be merged in the meantime?
>>
>> friendly ping :)
>>
>> With c5c2b11894f4 ("drm/i915: Warn against variable length arrays")
>> clang raises plenty of vla warnings about
>> __compiletime_error_fallback() in the i915 driver. Would be great to
>> get rid of those without having to revert that commit.
>
> I've been meaning to follow up on this, thanks Matthias. I too would
> really like this patch.
Adding Greg to the thread. Between Masahiro's detailed commit log and
the Clang-familiar reviewers, I think this should land for 4.19 (as
part of the other Clang-sanity patches that are already in 4.19). This
has no impact on gcc now that we're requiring 4.6+.
https://lore.kernel.org/patchwork/patch/977668/
-Kees
--
Kees Cook
Pixel Security
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Sep 26, 2018 at 11:00 AM Matthias Kaehlcke wrote:
>
> On Fri, Aug 31, 2018 at 09:46:02AM -0700, Nick Desaulniers wrote:
> > On Tue, Aug 28, 2018 at 4:00 PM Nick Desaulniers
> > wrote:
> > >
> > > On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos
> > > wrote:
> > > >
> > > > Hello Nick,
> > > >
> > > > On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> > > > >>> Let's give up __compiletime_assert_fallback(). This commit does not
> > > > >>> change the current behavior since it just rips off the useless code.
> > > > >> Clang is not the only target audience of
> > > > >> __compiletime_assert_fallback(). Instead of ripping out something
> > > > >> that
> > > > >> may benefit builds with gcc 4.2 and earlier, why not override its
> > > > > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> > > > > version to 4.6") that gcc < 4.6 is irrelevant.
> > > >
> > > > Ah, I guess I'm not keeping up, that's wonderful news! Considering that
> > > > I guess I would be OK with its removal, but I still think it would be
> > > > better if a similar mechanism to break the Clang build could be found.
> > >
> > > I'm consulting with our best language lawyers to see what combinations
> > > of _Static_assert and __builtin_constant_p would do the trick.
> >
> > Linus,
> > Can this patch be merged in the meantime?
>
> friendly ping :)
>
> With c5c2b11894f4 ("drm/i915: Warn against variable length arrays")
> clang raises plenty of vla warnings about
> __compiletime_error_fallback() in the i915 driver. Would be great to
> get rid of those without having to revert that commit.
I've been meaning to follow up on this, thanks Matthias. I too would
really like this patch.
--
Thanks,
~Nick Desaulniers
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Wed, Sep 26, 2018 at 11:00 AM Matthias Kaehlcke wrote:
>
> On Fri, Aug 31, 2018 at 09:46:02AM -0700, Nick Desaulniers wrote:
> > On Tue, Aug 28, 2018 at 4:00 PM Nick Desaulniers
> > wrote:
> > >
> > > On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos
> > > wrote:
> > > >
> > > > Hello Nick,
> > > >
> > > > On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> > > > >>> Let's give up __compiletime_assert_fallback(). This commit does not
> > > > >>> change the current behavior since it just rips off the useless code.
> > > > >> Clang is not the only target audience of
> > > > >> __compiletime_assert_fallback(). Instead of ripping out something
> > > > >> that
> > > > >> may benefit builds with gcc 4.2 and earlier, why not override its
> > > > > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> > > > > version to 4.6") that gcc < 4.6 is irrelevant.
> > > >
> > > > Ah, I guess I'm not keeping up, that's wonderful news! Considering that
> > > > I guess I would be OK with its removal, but I still think it would be
> > > > better if a similar mechanism to break the Clang build could be found.
> > >
> > > I'm consulting with our best language lawyers to see what combinations
> > > of _Static_assert and __builtin_constant_p would do the trick.
> >
> > Linus,
> > Can this patch be merged in the meantime?
>
> friendly ping :)
>
> With c5c2b11894f4 ("drm/i915: Warn against variable length arrays")
> clang raises plenty of vla warnings about
> __compiletime_error_fallback() in the i915 driver. Would be great to
> get rid of those without having to revert that commit.
I've been meaning to follow up on this, thanks Matthias. I too would
really like this patch.
--
Thanks,
~Nick Desaulniers
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Fri, Aug 31, 2018 at 09:46:02AM -0700, Nick Desaulniers wrote:
> On Tue, Aug 28, 2018 at 4:00 PM Nick Desaulniers
> wrote:
> >
> > On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos
> > wrote:
> > >
> > > Hello Nick,
> > >
> > > On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> > > >>> Let's give up __compiletime_assert_fallback(). This commit does not
> > > >>> change the current behavior since it just rips off the useless code.
> > > >> Clang is not the only target audience of
> > > >> __compiletime_assert_fallback(). Instead of ripping out something that
> > > >> may benefit builds with gcc 4.2 and earlier, why not override its
> > > > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> > > > version to 4.6") that gcc < 4.6 is irrelevant.
> > >
> > > Ah, I guess I'm not keeping up, that's wonderful news! Considering that
> > > I guess I would be OK with its removal, but I still think it would be
> > > better if a similar mechanism to break the Clang build could be found.
> >
> > I'm consulting with our best language lawyers to see what combinations
> > of _Static_assert and __builtin_constant_p would do the trick.
>
> Linus,
> Can this patch be merged in the meantime?
friendly ping :)
With c5c2b11894f4 ("drm/i915: Warn against variable length arrays")
clang raises plenty of vla warnings about
__compiletime_error_fallback() in the i915 driver. Would be great to
get rid of those without having to revert that commit.
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Fri, Aug 31, 2018 at 09:46:02AM -0700, Nick Desaulniers wrote:
> On Tue, Aug 28, 2018 at 4:00 PM Nick Desaulniers
> wrote:
> >
> > On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos
> > wrote:
> > >
> > > Hello Nick,
> > >
> > > On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> > > >>> Let's give up __compiletime_assert_fallback(). This commit does not
> > > >>> change the current behavior since it just rips off the useless code.
> > > >> Clang is not the only target audience of
> > > >> __compiletime_assert_fallback(). Instead of ripping out something that
> > > >> may benefit builds with gcc 4.2 and earlier, why not override its
> > > > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> > > > version to 4.6") that gcc < 4.6 is irrelevant.
> > >
> > > Ah, I guess I'm not keeping up, that's wonderful news! Considering that
> > > I guess I would be OK with its removal, but I still think it would be
> > > better if a similar mechanism to break the Clang build could be found.
> >
> > I'm consulting with our best language lawyers to see what combinations
> > of _Static_assert and __builtin_constant_p would do the trick.
>
> Linus,
> Can this patch be merged in the meantime?
friendly ping :)
With c5c2b11894f4 ("drm/i915: Warn against variable length arrays")
clang raises plenty of vla warnings about
__compiletime_error_fallback() in the i915 driver. Would be great to
get rid of those without having to revert that commit.
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Tue, Aug 28, 2018 at 4:00 PM Nick Desaulniers
wrote:
>
> On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos wrote:
> >
> > Hello Nick,
> >
> > On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> > >>> Let's give up __compiletime_assert_fallback(). This commit does not
> > >>> change the current behavior since it just rips off the useless code.
> > >> Clang is not the only target audience of
> > >> __compiletime_assert_fallback(). Instead of ripping out something that
> > >> may benefit builds with gcc 4.2 and earlier, why not override its
> > > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> > > version to 4.6") that gcc < 4.6 is irrelevant.
> >
> > Ah, I guess I'm not keeping up, that's wonderful news! Considering that
> > I guess I would be OK with its removal, but I still think it would be
> > better if a similar mechanism to break the Clang build could be found.
>
> I'm consulting with our best language lawyers to see what combinations
> of _Static_assert and __builtin_constant_p would do the trick.
Linus,
Can this patch be merged in the meantime?
--
Thanks,
~Nick Desaulniers
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Tue, Aug 28, 2018 at 4:00 PM Nick Desaulniers
wrote:
>
> On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos wrote:
> >
> > Hello Nick,
> >
> > On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> > >>> Let's give up __compiletime_assert_fallback(). This commit does not
> > >>> change the current behavior since it just rips off the useless code.
> > >> Clang is not the only target audience of
> > >> __compiletime_assert_fallback(). Instead of ripping out something that
> > >> may benefit builds with gcc 4.2 and earlier, why not override its
> > > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> > > version to 4.6") that gcc < 4.6 is irrelevant.
> >
> > Ah, I guess I'm not keeping up, that's wonderful news! Considering that
> > I guess I would be OK with its removal, but I still think it would be
> > better if a similar mechanism to break the Clang build could be found.
>
> I'm consulting with our best language lawyers to see what combinations
> of _Static_assert and __builtin_constant_p would do the trick.
Linus,
Can this patch be merged in the meantime?
--
Thanks,
~Nick Desaulniers
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos wrote:
>
> Hello Nick,
>
> On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> >>> Let's give up __compiletime_assert_fallback(). This commit does not
> >>> change the current behavior since it just rips off the useless code.
> >> Clang is not the only target audience of
> >> __compiletime_assert_fallback(). Instead of ripping out something that
> >> may benefit builds with gcc 4.2 and earlier, why not override its
> > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> > version to 4.6") that gcc < 4.6 is irrelevant.
>
> Ah, I guess I'm not keeping up, that's wonderful news! Considering that
> I guess I would be OK with its removal, but I still think it would be
> better if a similar mechanism to break the Clang build could be found.
I'm consulting with our best language lawyers to see what combinations
of _Static_assert and __builtin_constant_p would do the trick.
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos wrote:
>
> Hello Nick,
>
> On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> >>> Let's give up __compiletime_assert_fallback(). This commit does not
> >>> change the current behavior since it just rips off the useless code.
> >> Clang is not the only target audience of
> >> __compiletime_assert_fallback(). Instead of ripping out something that
> >> may benefit builds with gcc 4.2 and earlier, why not override its
> > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> > version to 4.6") that gcc < 4.6 is irrelevant.
>
> Ah, I guess I'm not keeping up, that's wonderful news! Considering that
> I guess I would be OK with its removal, but I still think it would be
> better if a similar mechanism to break the Clang build could be found.
I'm consulting with our best language lawyers to see what combinations
of _Static_assert and __builtin_constant_p would do the trick.
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
2018-08-28 19:55 GMT+09:00 Arnd Bergmann : > On Mon, Aug 27, 2018 at 10:44 PM Daniel Santos > wrote: >> >> On 08/27/2018 03:09 PM, Nick Desaulniers wrote: >> >> Now we're back to the question of "what do you mean by 'constant'"? If >> >> you mean a C constant expression (as defined in the C standard) than >> >> almost none of this code fits that criteria. For these compile-time >> >> assertions to work, we are concerned with the data flow analysis and >> >> constant propagation performed by the compiler during optimization. You >> >> will notice in include/linux/compiler.h that __compiletime_assert is a >> >> no-op when __OPTIMIZE__ is not defined. >> > Depending on optimizations for static assertions sounds problematic. >> >> (with my best Palpatine voice) It is unavoidable. >> >> Actually it's theoretically possible, but the compiler would have to do >> something akin to copying it's control flow graph et. al, run -O2-ish >> optimizations, perform the static assertions and then throw away the >> optimized control flow graph and emit code based upon the original. > > In the context of the kernel, compiling with anything less than -O2 or > -Os is not an issue, we don't do it anyway. -O0 never worked, and > AFAICT we only build one file with -O1, but that is something we can do > away with as well: > > from fs/reiserfs/Makefile: > # gcc -O2 (the kernel default) is overaggressive on ppc32 when many inline > # functions are used. This causes the compiler to advance the stack > # pointer out of the available stack space, corrupting kernel space, > # and causing a panic. Since this behavior only affects ppc32, this ifeq > # will work around it. If any other architecture displays this behavior, > # add it here. > ccflags-$(CONFIG_PPC32) := $(call cc-ifversion, -lt, 0400, -O1) > > Arnd Recently, I sent out patches to remove redundant GCC version checks, including this one. https://lore.kernel.org/patchwork/patch/977808/ I do not know who is maintaining reiserfs, though. -- Best Regards Masahiro Yamada
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
2018-08-28 19:55 GMT+09:00 Arnd Bergmann : > On Mon, Aug 27, 2018 at 10:44 PM Daniel Santos > wrote: >> >> On 08/27/2018 03:09 PM, Nick Desaulniers wrote: >> >> Now we're back to the question of "what do you mean by 'constant'"? If >> >> you mean a C constant expression (as defined in the C standard) than >> >> almost none of this code fits that criteria. For these compile-time >> >> assertions to work, we are concerned with the data flow analysis and >> >> constant propagation performed by the compiler during optimization. You >> >> will notice in include/linux/compiler.h that __compiletime_assert is a >> >> no-op when __OPTIMIZE__ is not defined. >> > Depending on optimizations for static assertions sounds problematic. >> >> (with my best Palpatine voice) It is unavoidable. >> >> Actually it's theoretically possible, but the compiler would have to do >> something akin to copying it's control flow graph et. al, run -O2-ish >> optimizations, perform the static assertions and then throw away the >> optimized control flow graph and emit code based upon the original. > > In the context of the kernel, compiling with anything less than -O2 or > -Os is not an issue, we don't do it anyway. -O0 never worked, and > AFAICT we only build one file with -O1, but that is something we can do > away with as well: > > from fs/reiserfs/Makefile: > # gcc -O2 (the kernel default) is overaggressive on ppc32 when many inline > # functions are used. This causes the compiler to advance the stack > # pointer out of the available stack space, corrupting kernel space, > # and causing a panic. Since this behavior only affects ppc32, this ifeq > # will work around it. If any other architecture displays this behavior, > # add it here. > ccflags-$(CONFIG_PPC32) := $(call cc-ifversion, -lt, 0400, -O1) > > Arnd Recently, I sent out patches to remove redundant GCC version checks, including this one. https://lore.kernel.org/patchwork/patch/977808/ I do not know who is maintaining reiserfs, though. -- Best Regards Masahiro Yamada
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Mon, Aug 27, 2018 at 10:44 PM Daniel Santos wrote: > > On 08/27/2018 03:09 PM, Nick Desaulniers wrote: > >> Now we're back to the question of "what do you mean by 'constant'"? If > >> you mean a C constant expression (as defined in the C standard) than > >> almost none of this code fits that criteria. For these compile-time > >> assertions to work, we are concerned with the data flow analysis and > >> constant propagation performed by the compiler during optimization. You > >> will notice in include/linux/compiler.h that __compiletime_assert is a > >> no-op when __OPTIMIZE__ is not defined. > > Depending on optimizations for static assertions sounds problematic. > > (with my best Palpatine voice) It is unavoidable. > > Actually it's theoretically possible, but the compiler would have to do > something akin to copying it's control flow graph et. al, run -O2-ish > optimizations, perform the static assertions and then throw away the > optimized control flow graph and emit code based upon the original. In the context of the kernel, compiling with anything less than -O2 or -Os is not an issue, we don't do it anyway. -O0 never worked, and AFAICT we only build one file with -O1, but that is something we can do away with as well: from fs/reiserfs/Makefile: # gcc -O2 (the kernel default) is overaggressive on ppc32 when many inline # functions are used. This causes the compiler to advance the stack # pointer out of the available stack space, corrupting kernel space, # and causing a panic. Since this behavior only affects ppc32, this ifeq # will work around it. If any other architecture displays this behavior, # add it here. ccflags-$(CONFIG_PPC32) := $(call cc-ifversion, -lt, 0400, -O1) Arnd
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Mon, Aug 27, 2018 at 10:44 PM Daniel Santos wrote: > > On 08/27/2018 03:09 PM, Nick Desaulniers wrote: > >> Now we're back to the question of "what do you mean by 'constant'"? If > >> you mean a C constant expression (as defined in the C standard) than > >> almost none of this code fits that criteria. For these compile-time > >> assertions to work, we are concerned with the data flow analysis and > >> constant propagation performed by the compiler during optimization. You > >> will notice in include/linux/compiler.h that __compiletime_assert is a > >> no-op when __OPTIMIZE__ is not defined. > > Depending on optimizations for static assertions sounds problematic. > > (with my best Palpatine voice) It is unavoidable. > > Actually it's theoretically possible, but the compiler would have to do > something akin to copying it's control flow graph et. al, run -O2-ish > optimizations, perform the static assertions and then throw away the > optimized control flow graph and emit code based upon the original. In the context of the kernel, compiling with anything less than -O2 or -Os is not an issue, we don't do it anyway. -O0 never worked, and AFAICT we only build one file with -O1, but that is something we can do away with as well: from fs/reiserfs/Makefile: # gcc -O2 (the kernel default) is overaggressive on ppc32 when many inline # functions are used. This causes the compiler to advance the stack # pointer out of the available stack space, corrupting kernel space, # and causing a panic. Since this behavior only affects ppc32, this ifeq # will work around it. If any other architecture displays this behavior, # add it here. ccflags-$(CONFIG_PPC32) := $(call cc-ifversion, -lt, 0400, -O1) Arnd
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos wrote:
>
> Hello Nick,
>
> On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> >> Now we're back to the question of "what do you mean by 'constant'"? If
> >> you mean a C constant expression (as defined in the C standard) than
> >> almost none of this code fits that criteria. For these compile-time
> >> assertions to work, we are concerned with the data flow analysis and
> >> constant propagation performed by the compiler during optimization. You
> >> will notice in include/linux/compiler.h that __compiletime_assert is a
> >> no-op when __OPTIMIZE__ is not defined.
> > Depending on optimizations for static assertions sounds problematic.
>
> (with my best Palpatine voice) It is unavoidable.
LOL
>
> Actually it's theoretically possible, but the compiler would have to do
> something akin to copying it's control flow graph et. al, run -O2-ish
> optimizations, perform the static assertions and then throw away the
> optimized control flow graph and emit code based upon the original.
>
>
> >>> As the comment in
> >>> says, GCC (and Clang as well) only emits the error for obvious cases.
> >>>
> >>> When '__cond' is a variable,
> >>>
> >>> ((void)sizeof(char[1 - 2 * __cond]))
> >>>
> >>> ... is not obvious for the compiler to know the array size is negative.
> >>>
> >>> Reverting that commit would break BUILD_BUG() because negative-size-array
> >>> is evaluated before the code is optimized out.
> >>>
> >>> Let's give up __compiletime_assert_fallback(). This commit does not
> >>> change the current behavior since it just rips off the useless code.
> >> Clang is not the only target audience of
> >> __compiletime_assert_fallback(). Instead of ripping out something that
> >> may benefit builds with gcc 4.2 and earlier, why not override its
> > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> > version to 4.6") that gcc < 4.6 is irrelevant.
>
> Ah, I guess I'm not keeping up, that's wonderful news! Considering that
> I guess I would be OK with its removal, but I still think it would be
> better if a similar mechanism to break the Clang build could be found.
>
> >> definition in compiler-clang.h with something that will break the build
> >> for Clang? It would need an #ifndef __compiletime_error_fallback here
> >> though.
> >>
> >>> Signed-off-by: Masahiro Yamada
> >>> Reviewed-by: Kees Cook
> >>> Reviewed-by: Nick Desaulniers
> >>> ---
> >>>
> >>> Changes in v2:
> >>> - Rebase
> >>>
> >>> include/linux/compiler.h | 17 +
> >>> 1 file changed, 1 insertion(+), 16 deletions(-)
> >>>
> >>> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> >>> index 681d866..87c776c 100644
> >>> --- a/include/linux/compiler.h
> >>> +++ b/include/linux/compiler.h
> >>> @@ -314,29 +314,14 @@ static inline void *offset_to_ptr(const int *off)
> >>> #endif
> >>> #ifndef __compiletime_error
> >>> # define __compiletime_error(message)
> >>> -/*
> >>> - * Sparse complains of variable sized arrays due to the temporary
> >>> variable in
> >>> - * __compiletime_assert. Unfortunately we can't just expand it out to
> >>> make
> >>> - * sparse see a constant array size without breaking compiletime_assert
> >>> on old
> >>> - * versions of GCC (e.g. 4.2.4), so hide the array from sparse
> >>> altogether.
> >>> - */
> >>> -# ifndef __CHECKER__
> >>> -# define __compiletime_error_fallback(condition) \
> >>> - do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
> >>> -# endif
> >>> -#endif
> >>> -#ifndef __compiletime_error_fallback
> >>> -# define __compiletime_error_fallback(condition) do { } while (0)
> >>> #endif
> >>>
> >>> #ifdef __OPTIMIZE__
> >>> # define __compiletime_assert(condition, msg, prefix, suffix)
> >>> \
> >>> do {\
> >>> - int __cond = !(condition); \
> >>> extern void prefix ## suffix(void)
> >>> __compiletime_error(msg); \
> >>> - if (__cond) \
> >>> + if (!(condition)) \
> >>> prefix ## suffix(); \
> >>> - __compiletime_error_fallback(__cond); \
> >>> } while (0)
> >>> #else
> >>> # define __compiletime_assert(condition, msg, prefix, suffix) do { }
> >>> while (0)
> >> To give any more meaningful feedback I think I will need to experiment
> >> with Clang, older GCC versions and icc. It occurred to me that I should
> >> probably clean up and publish my __builtin_constant_p test program and
> >> also generate results for more recent compilers. I can extend it to
> >> test various negative sized array constructs and it could help inform
> >> this decision.
> >>
> >> IMO, the most ideal solution would be a set of C2x (or future)
> >> extensions providing something similar
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos wrote:
>
> Hello Nick,
>
> On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> >> Now we're back to the question of "what do you mean by 'constant'"? If
> >> you mean a C constant expression (as defined in the C standard) than
> >> almost none of this code fits that criteria. For these compile-time
> >> assertions to work, we are concerned with the data flow analysis and
> >> constant propagation performed by the compiler during optimization. You
> >> will notice in include/linux/compiler.h that __compiletime_assert is a
> >> no-op when __OPTIMIZE__ is not defined.
> > Depending on optimizations for static assertions sounds problematic.
>
> (with my best Palpatine voice) It is unavoidable.
LOL
>
> Actually it's theoretically possible, but the compiler would have to do
> something akin to copying it's control flow graph et. al, run -O2-ish
> optimizations, perform the static assertions and then throw away the
> optimized control flow graph and emit code based upon the original.
>
>
> >>> As the comment in
> >>> says, GCC (and Clang as well) only emits the error for obvious cases.
> >>>
> >>> When '__cond' is a variable,
> >>>
> >>> ((void)sizeof(char[1 - 2 * __cond]))
> >>>
> >>> ... is not obvious for the compiler to know the array size is negative.
> >>>
> >>> Reverting that commit would break BUILD_BUG() because negative-size-array
> >>> is evaluated before the code is optimized out.
> >>>
> >>> Let's give up __compiletime_assert_fallback(). This commit does not
> >>> change the current behavior since it just rips off the useless code.
> >> Clang is not the only target audience of
> >> __compiletime_assert_fallback(). Instead of ripping out something that
> >> may benefit builds with gcc 4.2 and earlier, why not override its
> > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> > version to 4.6") that gcc < 4.6 is irrelevant.
>
> Ah, I guess I'm not keeping up, that's wonderful news! Considering that
> I guess I would be OK with its removal, but I still think it would be
> better if a similar mechanism to break the Clang build could be found.
>
> >> definition in compiler-clang.h with something that will break the build
> >> for Clang? It would need an #ifndef __compiletime_error_fallback here
> >> though.
> >>
> >>> Signed-off-by: Masahiro Yamada
> >>> Reviewed-by: Kees Cook
> >>> Reviewed-by: Nick Desaulniers
> >>> ---
> >>>
> >>> Changes in v2:
> >>> - Rebase
> >>>
> >>> include/linux/compiler.h | 17 +
> >>> 1 file changed, 1 insertion(+), 16 deletions(-)
> >>>
> >>> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> >>> index 681d866..87c776c 100644
> >>> --- a/include/linux/compiler.h
> >>> +++ b/include/linux/compiler.h
> >>> @@ -314,29 +314,14 @@ static inline void *offset_to_ptr(const int *off)
> >>> #endif
> >>> #ifndef __compiletime_error
> >>> # define __compiletime_error(message)
> >>> -/*
> >>> - * Sparse complains of variable sized arrays due to the temporary
> >>> variable in
> >>> - * __compiletime_assert. Unfortunately we can't just expand it out to
> >>> make
> >>> - * sparse see a constant array size without breaking compiletime_assert
> >>> on old
> >>> - * versions of GCC (e.g. 4.2.4), so hide the array from sparse
> >>> altogether.
> >>> - */
> >>> -# ifndef __CHECKER__
> >>> -# define __compiletime_error_fallback(condition) \
> >>> - do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
> >>> -# endif
> >>> -#endif
> >>> -#ifndef __compiletime_error_fallback
> >>> -# define __compiletime_error_fallback(condition) do { } while (0)
> >>> #endif
> >>>
> >>> #ifdef __OPTIMIZE__
> >>> # define __compiletime_assert(condition, msg, prefix, suffix)
> >>> \
> >>> do {\
> >>> - int __cond = !(condition); \
> >>> extern void prefix ## suffix(void)
> >>> __compiletime_error(msg); \
> >>> - if (__cond) \
> >>> + if (!(condition)) \
> >>> prefix ## suffix(); \
> >>> - __compiletime_error_fallback(__cond); \
> >>> } while (0)
> >>> #else
> >>> # define __compiletime_assert(condition, msg, prefix, suffix) do { }
> >>> while (0)
> >> To give any more meaningful feedback I think I will need to experiment
> >> with Clang, older GCC versions and icc. It occurred to me that I should
> >> probably clean up and publish my __builtin_constant_p test program and
> >> also generate results for more recent compilers. I can extend it to
> >> test various negative sized array constructs and it could help inform
> >> this decision.
> >>
> >> IMO, the most ideal solution would be a set of C2x (or future)
> >> extensions providing something similar
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
Hello Nick,
On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
>> Now we're back to the question of "what do you mean by 'constant'"? If
>> you mean a C constant expression (as defined in the C standard) than
>> almost none of this code fits that criteria. For these compile-time
>> assertions to work, we are concerned with the data flow analysis and
>> constant propagation performed by the compiler during optimization. You
>> will notice in include/linux/compiler.h that __compiletime_assert is a
>> no-op when __OPTIMIZE__ is not defined.
> Depending on optimizations for static assertions sounds problematic.
(with my best Palpatine voice) It is unavoidable.
Actually it's theoretically possible, but the compiler would have to do
something akin to copying it's control flow graph et. al, run -O2-ish
optimizations, perform the static assertions and then throw away the
optimized control flow graph and emit code based upon the original.
>>> As the comment in
>>> says, GCC (and Clang as well) only emits the error for obvious cases.
>>>
>>> When '__cond' is a variable,
>>>
>>> ((void)sizeof(char[1 - 2 * __cond]))
>>>
>>> ... is not obvious for the compiler to know the array size is negative.
>>>
>>> Reverting that commit would break BUILD_BUG() because negative-size-array
>>> is evaluated before the code is optimized out.
>>>
>>> Let's give up __compiletime_assert_fallback(). This commit does not
>>> change the current behavior since it just rips off the useless code.
>> Clang is not the only target audience of
>> __compiletime_assert_fallback(). Instead of ripping out something that
>> may benefit builds with gcc 4.2 and earlier, why not override its
> Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> version to 4.6") that gcc < 4.6 is irrelevant.
Ah, I guess I'm not keeping up, that's wonderful news! Considering that
I guess I would be OK with its removal, but I still think it would be
better if a similar mechanism to break the Clang build could be found.
>> definition in compiler-clang.h with something that will break the build
>> for Clang? It would need an #ifndef __compiletime_error_fallback here
>> though.
>>
>>> Signed-off-by: Masahiro Yamada
>>> Reviewed-by: Kees Cook
>>> Reviewed-by: Nick Desaulniers
>>> ---
>>>
>>> Changes in v2:
>>> - Rebase
>>>
>>> include/linux/compiler.h | 17 +
>>> 1 file changed, 1 insertion(+), 16 deletions(-)
>>>
>>> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
>>> index 681d866..87c776c 100644
>>> --- a/include/linux/compiler.h
>>> +++ b/include/linux/compiler.h
>>> @@ -314,29 +314,14 @@ static inline void *offset_to_ptr(const int *off)
>>> #endif
>>> #ifndef __compiletime_error
>>> # define __compiletime_error(message)
>>> -/*
>>> - * Sparse complains of variable sized arrays due to the temporary variable
>>> in
>>> - * __compiletime_assert. Unfortunately we can't just expand it out to make
>>> - * sparse see a constant array size without breaking compiletime_assert on
>>> old
>>> - * versions of GCC (e.g. 4.2.4), so hide the array from sparse altogether.
>>> - */
>>> -# ifndef __CHECKER__
>>> -# define __compiletime_error_fallback(condition) \
>>> - do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
>>> -# endif
>>> -#endif
>>> -#ifndef __compiletime_error_fallback
>>> -# define __compiletime_error_fallback(condition) do { } while (0)
>>> #endif
>>>
>>> #ifdef __OPTIMIZE__
>>> # define __compiletime_assert(condition, msg, prefix, suffix)
>>> \
>>> do {\
>>> - int __cond = !(condition); \
>>> extern void prefix ## suffix(void) __compiletime_error(msg); \
>>> - if (__cond) \
>>> + if (!(condition)) \
>>> prefix ## suffix(); \
>>> - __compiletime_error_fallback(__cond); \
>>> } while (0)
>>> #else
>>> # define __compiletime_assert(condition, msg, prefix, suffix) do { } while
>>> (0)
>> To give any more meaningful feedback I think I will need to experiment
>> with Clang, older GCC versions and icc. It occurred to me that I should
>> probably clean up and publish my __builtin_constant_p test program and
>> also generate results for more recent compilers. I can extend it to
>> test various negative sized array constructs and it could help inform
>> this decision.
>>
>> IMO, the most ideal solution would be a set of C2x (or future)
>> extensions providing something similar to C++'s constexpr, GCC's
>> __builtin_constant_p and our BUILD_BUG_ON. This would cross deeply into
> Note that __builtin_constant_p is a wild beast with lots of edge
> cases, and dependencies on compiler and optimization level. I'm
> trying to sort out some of these differences right
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
Hello Nick,
On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
>> Now we're back to the question of "what do you mean by 'constant'"? If
>> you mean a C constant expression (as defined in the C standard) than
>> almost none of this code fits that criteria. For these compile-time
>> assertions to work, we are concerned with the data flow analysis and
>> constant propagation performed by the compiler during optimization. You
>> will notice in include/linux/compiler.h that __compiletime_assert is a
>> no-op when __OPTIMIZE__ is not defined.
> Depending on optimizations for static assertions sounds problematic.
(with my best Palpatine voice) It is unavoidable.
Actually it's theoretically possible, but the compiler would have to do
something akin to copying it's control flow graph et. al, run -O2-ish
optimizations, perform the static assertions and then throw away the
optimized control flow graph and emit code based upon the original.
>>> As the comment in
>>> says, GCC (and Clang as well) only emits the error for obvious cases.
>>>
>>> When '__cond' is a variable,
>>>
>>> ((void)sizeof(char[1 - 2 * __cond]))
>>>
>>> ... is not obvious for the compiler to know the array size is negative.
>>>
>>> Reverting that commit would break BUILD_BUG() because negative-size-array
>>> is evaluated before the code is optimized out.
>>>
>>> Let's give up __compiletime_assert_fallback(). This commit does not
>>> change the current behavior since it just rips off the useless code.
>> Clang is not the only target audience of
>> __compiletime_assert_fallback(). Instead of ripping out something that
>> may benefit builds with gcc 4.2 and earlier, why not override its
> Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> version to 4.6") that gcc < 4.6 is irrelevant.
Ah, I guess I'm not keeping up, that's wonderful news! Considering that
I guess I would be OK with its removal, but I still think it would be
better if a similar mechanism to break the Clang build could be found.
>> definition in compiler-clang.h with something that will break the build
>> for Clang? It would need an #ifndef __compiletime_error_fallback here
>> though.
>>
>>> Signed-off-by: Masahiro Yamada
>>> Reviewed-by: Kees Cook
>>> Reviewed-by: Nick Desaulniers
>>> ---
>>>
>>> Changes in v2:
>>> - Rebase
>>>
>>> include/linux/compiler.h | 17 +
>>> 1 file changed, 1 insertion(+), 16 deletions(-)
>>>
>>> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
>>> index 681d866..87c776c 100644
>>> --- a/include/linux/compiler.h
>>> +++ b/include/linux/compiler.h
>>> @@ -314,29 +314,14 @@ static inline void *offset_to_ptr(const int *off)
>>> #endif
>>> #ifndef __compiletime_error
>>> # define __compiletime_error(message)
>>> -/*
>>> - * Sparse complains of variable sized arrays due to the temporary variable
>>> in
>>> - * __compiletime_assert. Unfortunately we can't just expand it out to make
>>> - * sparse see a constant array size without breaking compiletime_assert on
>>> old
>>> - * versions of GCC (e.g. 4.2.4), so hide the array from sparse altogether.
>>> - */
>>> -# ifndef __CHECKER__
>>> -# define __compiletime_error_fallback(condition) \
>>> - do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
>>> -# endif
>>> -#endif
>>> -#ifndef __compiletime_error_fallback
>>> -# define __compiletime_error_fallback(condition) do { } while (0)
>>> #endif
>>>
>>> #ifdef __OPTIMIZE__
>>> # define __compiletime_assert(condition, msg, prefix, suffix)
>>> \
>>> do {\
>>> - int __cond = !(condition); \
>>> extern void prefix ## suffix(void) __compiletime_error(msg); \
>>> - if (__cond) \
>>> + if (!(condition)) \
>>> prefix ## suffix(); \
>>> - __compiletime_error_fallback(__cond); \
>>> } while (0)
>>> #else
>>> # define __compiletime_assert(condition, msg, prefix, suffix) do { } while
>>> (0)
>> To give any more meaningful feedback I think I will need to experiment
>> with Clang, older GCC versions and icc. It occurred to me that I should
>> probably clean up and publish my __builtin_constant_p test program and
>> also generate results for more recent compilers. I can extend it to
>> test various negative sized array constructs and it could help inform
>> this decision.
>>
>> IMO, the most ideal solution would be a set of C2x (or future)
>> extensions providing something similar to C++'s constexpr, GCC's
>> __builtin_constant_p and our BUILD_BUG_ON. This would cross deeply into
> Note that __builtin_constant_p is a wild beast with lots of edge
> cases, and dependencies on compiler and optimization level. I'm
> trying to sort out some of these differences right
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Mon, Aug 27, 2018 at 1:05 PM Daniel Santos wrote:
>
> Hello Masahiro,
>
>
> On 08/25/2018 01:16 PM, Masahiro Yamada wrote:
> > __compiletime_assert_fallback() is supposed to stop building earlier
> > by using the negative-array-size method in case the compiler does not
> > support "error" attribute, but has never worked like that.
> >
> > You can simply try:
> >
> > BUILD_BUG_ON(1);
> >
> > GCC immediately terminates the build, but Clang does not report
> > anything because Clang does not support the "error" attribute now.
> > It will later fail at link time, but __compiletime_assert_fallback()
> > is not working at least.
> >
> > The root cause is commit 1d6a0d19c855 ("bug.h: prevent double evaluation
> > of `condition' in BUILD_BUG_ON").
>
> I didn't really think this particular patch was necessary, but it was
> requested that I eliminate double evaluation and I didn't feel like
> arguing it at the time. :) In my philosophy however, one should *never*
> use an expression with side effects in any type of assert.
>
> > Prior to that commit, BUILD_BUG_ON()
> > was checked by the negative-array-size method *and* the link-time trick.
> > Since that commit, the negative-array-size is not effective because
> > '__cond' is no longer constant.
>
> Now we're back to the question of "what do you mean by 'constant'"? If
> you mean a C constant expression (as defined in the C standard) than
> almost none of this code fits that criteria. For these compile-time
> assertions to work, we are concerned with the data flow analysis and
> constant propagation performed by the compiler during optimization. You
> will notice in include/linux/compiler.h that __compiletime_assert is a
> no-op when __OPTIMIZE__ is not defined.
Depending on optimizations for static assertions sounds problematic.
>
> > As the comment in
> > says, GCC (and Clang as well) only emits the error for obvious cases.
> >
> > When '__cond' is a variable,
> >
> > ((void)sizeof(char[1 - 2 * __cond]))
> >
> > ... is not obvious for the compiler to know the array size is negative.
> >
> > Reverting that commit would break BUILD_BUG() because negative-size-array
> > is evaluated before the code is optimized out.
> >
> > Let's give up __compiletime_assert_fallback(). This commit does not
> > change the current behavior since it just rips off the useless code.
>
> Clang is not the only target audience of
> __compiletime_assert_fallback(). Instead of ripping out something that
> may benefit builds with gcc 4.2 and earlier, why not override its
Note that with commit cafa0010cd51 ("Raise the minimum required gcc
version to 4.6") that gcc < 4.6 is irrelevant.
> definition in compiler-clang.h with something that will break the build
> for Clang? It would need an #ifndef __compiletime_error_fallback here
> though.
>
> > Signed-off-by: Masahiro Yamada
> > Reviewed-by: Kees Cook
> > Reviewed-by: Nick Desaulniers
> > ---
> >
> > Changes in v2:
> > - Rebase
> >
> > include/linux/compiler.h | 17 +
> > 1 file changed, 1 insertion(+), 16 deletions(-)
> >
> > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > index 681d866..87c776c 100644
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -314,29 +314,14 @@ static inline void *offset_to_ptr(const int *off)
> > #endif
> > #ifndef __compiletime_error
> > # define __compiletime_error(message)
> > -/*
> > - * Sparse complains of variable sized arrays due to the temporary variable
> > in
> > - * __compiletime_assert. Unfortunately we can't just expand it out to make
> > - * sparse see a constant array size without breaking compiletime_assert on
> > old
> > - * versions of GCC (e.g. 4.2.4), so hide the array from sparse altogether.
> > - */
> > -# ifndef __CHECKER__
> > -# define __compiletime_error_fallback(condition) \
> > - do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
> > -# endif
> > -#endif
> > -#ifndef __compiletime_error_fallback
> > -# define __compiletime_error_fallback(condition) do { } while (0)
> > #endif
> >
> > #ifdef __OPTIMIZE__
> > # define __compiletime_assert(condition, msg, prefix, suffix)
> > \
> > do {\
> > - int __cond = !(condition); \
> > extern void prefix ## suffix(void) __compiletime_error(msg); \
> > - if (__cond) \
> > + if (!(condition)) \
> > prefix ## suffix(); \
> > - __compiletime_error_fallback(__cond); \
> > } while (0)
> > #else
> > # define __compiletime_assert(condition, msg, prefix, suffix) do { } while
> > (0)
>
> To give any more meaningful feedback I think I will need to experiment
> with Clang, older GCC versions and icc. It occurred to me that I
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
On Mon, Aug 27, 2018 at 1:05 PM Daniel Santos wrote:
>
> Hello Masahiro,
>
>
> On 08/25/2018 01:16 PM, Masahiro Yamada wrote:
> > __compiletime_assert_fallback() is supposed to stop building earlier
> > by using the negative-array-size method in case the compiler does not
> > support "error" attribute, but has never worked like that.
> >
> > You can simply try:
> >
> > BUILD_BUG_ON(1);
> >
> > GCC immediately terminates the build, but Clang does not report
> > anything because Clang does not support the "error" attribute now.
> > It will later fail at link time, but __compiletime_assert_fallback()
> > is not working at least.
> >
> > The root cause is commit 1d6a0d19c855 ("bug.h: prevent double evaluation
> > of `condition' in BUILD_BUG_ON").
>
> I didn't really think this particular patch was necessary, but it was
> requested that I eliminate double evaluation and I didn't feel like
> arguing it at the time. :) In my philosophy however, one should *never*
> use an expression with side effects in any type of assert.
>
> > Prior to that commit, BUILD_BUG_ON()
> > was checked by the negative-array-size method *and* the link-time trick.
> > Since that commit, the negative-array-size is not effective because
> > '__cond' is no longer constant.
>
> Now we're back to the question of "what do you mean by 'constant'"? If
> you mean a C constant expression (as defined in the C standard) than
> almost none of this code fits that criteria. For these compile-time
> assertions to work, we are concerned with the data flow analysis and
> constant propagation performed by the compiler during optimization. You
> will notice in include/linux/compiler.h that __compiletime_assert is a
> no-op when __OPTIMIZE__ is not defined.
Depending on optimizations for static assertions sounds problematic.
>
> > As the comment in
> > says, GCC (and Clang as well) only emits the error for obvious cases.
> >
> > When '__cond' is a variable,
> >
> > ((void)sizeof(char[1 - 2 * __cond]))
> >
> > ... is not obvious for the compiler to know the array size is negative.
> >
> > Reverting that commit would break BUILD_BUG() because negative-size-array
> > is evaluated before the code is optimized out.
> >
> > Let's give up __compiletime_assert_fallback(). This commit does not
> > change the current behavior since it just rips off the useless code.
>
> Clang is not the only target audience of
> __compiletime_assert_fallback(). Instead of ripping out something that
> may benefit builds with gcc 4.2 and earlier, why not override its
Note that with commit cafa0010cd51 ("Raise the minimum required gcc
version to 4.6") that gcc < 4.6 is irrelevant.
> definition in compiler-clang.h with something that will break the build
> for Clang? It would need an #ifndef __compiletime_error_fallback here
> though.
>
> > Signed-off-by: Masahiro Yamada
> > Reviewed-by: Kees Cook
> > Reviewed-by: Nick Desaulniers
> > ---
> >
> > Changes in v2:
> > - Rebase
> >
> > include/linux/compiler.h | 17 +
> > 1 file changed, 1 insertion(+), 16 deletions(-)
> >
> > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > index 681d866..87c776c 100644
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -314,29 +314,14 @@ static inline void *offset_to_ptr(const int *off)
> > #endif
> > #ifndef __compiletime_error
> > # define __compiletime_error(message)
> > -/*
> > - * Sparse complains of variable sized arrays due to the temporary variable
> > in
> > - * __compiletime_assert. Unfortunately we can't just expand it out to make
> > - * sparse see a constant array size without breaking compiletime_assert on
> > old
> > - * versions of GCC (e.g. 4.2.4), so hide the array from sparse altogether.
> > - */
> > -# ifndef __CHECKER__
> > -# define __compiletime_error_fallback(condition) \
> > - do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
> > -# endif
> > -#endif
> > -#ifndef __compiletime_error_fallback
> > -# define __compiletime_error_fallback(condition) do { } while (0)
> > #endif
> >
> > #ifdef __OPTIMIZE__
> > # define __compiletime_assert(condition, msg, prefix, suffix)
> > \
> > do {\
> > - int __cond = !(condition); \
> > extern void prefix ## suffix(void) __compiletime_error(msg); \
> > - if (__cond) \
> > + if (!(condition)) \
> > prefix ## suffix(); \
> > - __compiletime_error_fallback(__cond); \
> > } while (0)
> > #else
> > # define __compiletime_assert(condition, msg, prefix, suffix) do { } while
> > (0)
>
> To give any more meaningful feedback I think I will need to experiment
> with Clang, older GCC versions and icc. It occurred to me that I
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
Hello Masahiro,
On 08/25/2018 01:16 PM, Masahiro Yamada wrote:
> __compiletime_assert_fallback() is supposed to stop building earlier
> by using the negative-array-size method in case the compiler does not
> support "error" attribute, but has never worked like that.
>
> You can simply try:
>
> BUILD_BUG_ON(1);
>
> GCC immediately terminates the build, but Clang does not report
> anything because Clang does not support the "error" attribute now.
> It will later fail at link time, but __compiletime_assert_fallback()
> is not working at least.
>
> The root cause is commit 1d6a0d19c855 ("bug.h: prevent double evaluation
> of `condition' in BUILD_BUG_ON").
I didn't really think this particular patch was necessary, but it was
requested that I eliminate double evaluation and I didn't feel like
arguing it at the time. :) In my philosophy however, one should *never*
use an expression with side effects in any type of assert.
> Prior to that commit, BUILD_BUG_ON()
> was checked by the negative-array-size method *and* the link-time trick.
> Since that commit, the negative-array-size is not effective because
> '__cond' is no longer constant.
Now we're back to the question of "what do you mean by 'constant'"? If
you mean a C constant expression (as defined in the C standard) than
almost none of this code fits that criteria. For these compile-time
assertions to work, we are concerned with the data flow analysis and
constant propagation performed by the compiler during optimization. You
will notice in include/linux/compiler.h that __compiletime_assert is a
no-op when __OPTIMIZE__ is not defined.
> As the comment in
> says, GCC (and Clang as well) only emits the error for obvious cases.
>
> When '__cond' is a variable,
>
> ((void)sizeof(char[1 - 2 * __cond]))
>
> ... is not obvious for the compiler to know the array size is negative.
>
> Reverting that commit would break BUILD_BUG() because negative-size-array
> is evaluated before the code is optimized out.
>
> Let's give up __compiletime_assert_fallback(). This commit does not
> change the current behavior since it just rips off the useless code.
Clang is not the only target audience of
__compiletime_assert_fallback(). Instead of ripping out something that
may benefit builds with gcc 4.2 and earlier, why not override its
definition in compiler-clang.h with something that will break the build
for Clang? It would need an #ifndef __compiletime_error_fallback here
though.
> Signed-off-by: Masahiro Yamada
> Reviewed-by: Kees Cook
> Reviewed-by: Nick Desaulniers
> ---
>
> Changes in v2:
> - Rebase
>
> include/linux/compiler.h | 17 +
> 1 file changed, 1 insertion(+), 16 deletions(-)
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 681d866..87c776c 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -314,29 +314,14 @@ static inline void *offset_to_ptr(const int *off)
> #endif
> #ifndef __compiletime_error
> # define __compiletime_error(message)
> -/*
> - * Sparse complains of variable sized arrays due to the temporary variable in
> - * __compiletime_assert. Unfortunately we can't just expand it out to make
> - * sparse see a constant array size without breaking compiletime_assert on
> old
> - * versions of GCC (e.g. 4.2.4), so hide the array from sparse altogether.
> - */
> -# ifndef __CHECKER__
> -# define __compiletime_error_fallback(condition) \
> - do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
> -# endif
> -#endif
> -#ifndef __compiletime_error_fallback
> -# define __compiletime_error_fallback(condition) do { } while (0)
> #endif
>
> #ifdef __OPTIMIZE__
> # define __compiletime_assert(condition, msg, prefix, suffix)
> \
> do {\
> - int __cond = !(condition); \
> extern void prefix ## suffix(void) __compiletime_error(msg); \
> - if (__cond) \
> + if (!(condition)) \
> prefix ## suffix(); \
> - __compiletime_error_fallback(__cond); \
> } while (0)
> #else
> # define __compiletime_assert(condition, msg, prefix, suffix) do { } while
> (0)
To give any more meaningful feedback I think I will need to experiment
with Clang, older GCC versions and icc. It occurred to me that I should
probably clean up and publish my __builtin_constant_p test program and
also generate results for more recent compilers. I can extend it to
test various negative sized array constructs and it could help inform
this decision.
IMO, the most ideal solution would be a set of C2x (or future)
extensions providing something similar to C++'s constexpr, GCC's
__builtin_constant_p and our BUILD_BUG_ON. This would cross deeply into
territory traditionally
Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()
Hello Masahiro,
On 08/25/2018 01:16 PM, Masahiro Yamada wrote:
> __compiletime_assert_fallback() is supposed to stop building earlier
> by using the negative-array-size method in case the compiler does not
> support "error" attribute, but has never worked like that.
>
> You can simply try:
>
> BUILD_BUG_ON(1);
>
> GCC immediately terminates the build, but Clang does not report
> anything because Clang does not support the "error" attribute now.
> It will later fail at link time, but __compiletime_assert_fallback()
> is not working at least.
>
> The root cause is commit 1d6a0d19c855 ("bug.h: prevent double evaluation
> of `condition' in BUILD_BUG_ON").
I didn't really think this particular patch was necessary, but it was
requested that I eliminate double evaluation and I didn't feel like
arguing it at the time. :) In my philosophy however, one should *never*
use an expression with side effects in any type of assert.
> Prior to that commit, BUILD_BUG_ON()
> was checked by the negative-array-size method *and* the link-time trick.
> Since that commit, the negative-array-size is not effective because
> '__cond' is no longer constant.
Now we're back to the question of "what do you mean by 'constant'"? If
you mean a C constant expression (as defined in the C standard) than
almost none of this code fits that criteria. For these compile-time
assertions to work, we are concerned with the data flow analysis and
constant propagation performed by the compiler during optimization. You
will notice in include/linux/compiler.h that __compiletime_assert is a
no-op when __OPTIMIZE__ is not defined.
> As the comment in
> says, GCC (and Clang as well) only emits the error for obvious cases.
>
> When '__cond' is a variable,
>
> ((void)sizeof(char[1 - 2 * __cond]))
>
> ... is not obvious for the compiler to know the array size is negative.
>
> Reverting that commit would break BUILD_BUG() because negative-size-array
> is evaluated before the code is optimized out.
>
> Let's give up __compiletime_assert_fallback(). This commit does not
> change the current behavior since it just rips off the useless code.
Clang is not the only target audience of
__compiletime_assert_fallback(). Instead of ripping out something that
may benefit builds with gcc 4.2 and earlier, why not override its
definition in compiler-clang.h with something that will break the build
for Clang? It would need an #ifndef __compiletime_error_fallback here
though.
> Signed-off-by: Masahiro Yamada
> Reviewed-by: Kees Cook
> Reviewed-by: Nick Desaulniers
> ---
>
> Changes in v2:
> - Rebase
>
> include/linux/compiler.h | 17 +
> 1 file changed, 1 insertion(+), 16 deletions(-)
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 681d866..87c776c 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -314,29 +314,14 @@ static inline void *offset_to_ptr(const int *off)
> #endif
> #ifndef __compiletime_error
> # define __compiletime_error(message)
> -/*
> - * Sparse complains of variable sized arrays due to the temporary variable in
> - * __compiletime_assert. Unfortunately we can't just expand it out to make
> - * sparse see a constant array size without breaking compiletime_assert on
> old
> - * versions of GCC (e.g. 4.2.4), so hide the array from sparse altogether.
> - */
> -# ifndef __CHECKER__
> -# define __compiletime_error_fallback(condition) \
> - do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
> -# endif
> -#endif
> -#ifndef __compiletime_error_fallback
> -# define __compiletime_error_fallback(condition) do { } while (0)
> #endif
>
> #ifdef __OPTIMIZE__
> # define __compiletime_assert(condition, msg, prefix, suffix)
> \
> do {\
> - int __cond = !(condition); \
> extern void prefix ## suffix(void) __compiletime_error(msg); \
> - if (__cond) \
> + if (!(condition)) \
> prefix ## suffix(); \
> - __compiletime_error_fallback(__cond); \
> } while (0)
> #else
> # define __compiletime_assert(condition, msg, prefix, suffix) do { } while
> (0)
To give any more meaningful feedback I think I will need to experiment
with Clang, older GCC versions and icc. It occurred to me that I should
probably clean up and publish my __builtin_constant_p test program and
also generate results for more recent compilers. I can extend it to
test various negative sized array constructs and it could help inform
this decision.
IMO, the most ideal solution would be a set of C2x (or future)
extensions providing something similar to C++'s constexpr, GCC's
__builtin_constant_p and our BUILD_BUG_ON. This would cross deeply into
territory traditionally
