Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20181009171401.14980-1-natechancellor@gmail.com>
In-Reply-To: <20181009171401.14980-1-natechancellor@gmail.com>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Tue, 9 Oct 2018 13:59:46 -0700
Message-ID: <CAKwvOdm-oiVvuX_QDjQ1QcO3+=XowHEnHgj3PyQ4hY=thrJGNA@mail.gmail.com>
Subject: Re: [PATCH] drm/i915: Convert _print_param to a macro
To:     Nathan Chancellor <natechancellor@gmail.com>
Cc:     jani.nikula@linux.intel.com, joonas.lahtinen@linux.intel.com,
        rodrigo.vivi@intel.com, intel-gfx@lists.freedesktop.org,
        dri-devel@lists.freedesktop.org,
        LKML <linux-kernel@vger.kernel.org>, lukas.bulwahn@gmail.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Oct 9, 2018 at 10:14 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> When building the kernel with Clang with defconfig and CONFIG_64BIT
> disabled, vmlinux fails to link because of the BUILD_BUG in
> _print_param.
>
> ld: drivers/gpu/drm/i915/i915_params.o: in function `i915_params_dump':
> i915_params.c:(.text+0x56): undefined reference to
> `__compiletime_assert_191'
>
> This function is semantically invalid unless the code is first inlined
> then constant folded, which doesn't work for Clang because semantic
> analysis happens before optimization/inlining. Converting this function
> to a macro avoids this problem and allows Clang to properly remove the
> BUILD_BUG during optimization.

Thanks Nathan for the patch.  To provide more context, Clang does
semantic analysis before optimization, where as GCC does these
together (IIUC).  So the above link error is from the naked
BUILD_BUG().  Clang can't evaluate the __builtin_strcmp's statically
until inlining has occurred, but that optimization happens after
semantic analysis.  To do the inlining before semantic analysis, we
MUST leverage the preprocessor, which runs before the compiler starts
doing semantic analysis.  I suspect this code is not valid for GCC
unless optimizations are enabled (the kernel only does compile with
optimizations turned on).  This change allows us to build this
translation unit with Clang.

Acked-by: Nick Desaulniers <ndesaulniers@google.com>
(Note: this is the change I suggested, so not sure whether Acked-by or
Reviewed-by is more appropriate).

>
> The output of 'objdump -D' is identically before and after this change
> for GCC regardless of if CONFIG_64BIT is set and allows Clang to link
> the kernel successfully with or without CONFIG_64BIT set.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/191
> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_params.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 295e981e4a39..a0f20b9b6f2d 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -174,22 +174,19 @@ i915_param_named(enable_dpcd_backlight, bool, 0600,
>  i915_param_named(enable_gvt, bool, 0400,
>         "Enable support for Intel GVT-g graphics virtualization host support(default:false)");
>
> -static __always_inline void _print_param(struct drm_printer *p,
> -                                        const char *name,
> -                                        const char *type,
> -                                        const void *x)
> -{
> -       if (!__builtin_strcmp(type, "bool"))
> -               drm_printf(p, "i915.%s=%s\n", name, yesno(*(const bool *)x));
> -       else if (!__builtin_strcmp(type, "int"))
> -               drm_printf(p, "i915.%s=%d\n", name, *(const int *)x);
> -       else if (!__builtin_strcmp(type, "unsigned int"))
> -               drm_printf(p, "i915.%s=%u\n", name, *(const unsigned int *)x);
> -       else if (!__builtin_strcmp(type, "char *"))
> -               drm_printf(p, "i915.%s=%s\n", name, *(const char **)x);
> -       else
> -               BUILD_BUG();
> -}
> +#define _print_param(p, name, type, x)                                        \
> +do {                                                                          \
> +       if (!__builtin_strcmp(type, "bool"))                                   \
> +               drm_printf(p, "i915.%s=%s\n", name, yesno(*(const bool *)x));  \
> +       else if (!__builtin_strcmp(type, "int"))                               \
> +               drm_printf(p, "i915.%s=%d\n", name, *(const int *)x);          \
> +       else if (!__builtin_strcmp(type, "unsigned int"))                      \
> +               drm_printf(p, "i915.%s=%u\n", name, *(const unsigned int *)x); \
> +       else if (!__builtin_strcmp(type, "char *"))                            \
> +               drm_printf(p, "i915.%s=%s\n", name, *(const char **)x);        \
> +       else                                                                   \
> +               BUILD_BUG();                                                   \
> +} while (0)
>
>  /**
>   * i915_params_dump - dump i915 modparams
> --
> 2.19.0
>


-- 
Thanks,
~Nick Desaulniers