* [PATCH] drm/i915: Convert _print_param to a macro
@ 2018-10-09 17:14 Nathan Chancellor
2018-10-09 20:59 ` Nick Desaulniers
` (6 more replies)
0 siblings, 7 replies; 15+ messages in thread
From: Nathan Chancellor @ 2018-10-09 17:14 UTC (permalink / raw)
To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi
Cc: intel-gfx, dri-devel, linux-kernel, Lukas Bulwahn,
Nick Desaulniers, Nathan Chancellor
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.
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Convert _print_param to a macro
2018-10-09 17:14 [PATCH] drm/i915: Convert _print_param to a macro Nathan Chancellor
@ 2018-10-09 20:59 ` Nick Desaulniers
2018-10-10 12:01 ` Jani Nikula
2018-10-10 9:34 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
` (5 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Nick Desaulniers @ 2018-10-09 20:59 UTC (permalink / raw)
To: Nathan Chancellor
Cc: jani.nikula, joonas.lahtinen, rodrigo.vivi, intel-gfx, dri-devel,
LKML, lukas.bulwahn
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Convert _print_param to a macro
2018-10-09 17:14 [PATCH] drm/i915: Convert _print_param to a macro Nathan Chancellor
2018-10-09 20:59 ` Nick Desaulniers
@ 2018-10-10 9:34 ` Patchwork
2018-10-10 9:50 ` ✓ Fi.CI.BAT: success " Patchwork
` (4 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2018-10-10 9:34 UTC (permalink / raw)
To: Nathan Chancellor; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Convert _print_param to a macro
URL : https://patchwork.freedesktop.org/series/50789/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
fb0e5c359f1a drm/i915: Convert _print_param to a macro
-:53: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'p' - possible side-effects?
#53: FILE: drivers/gpu/drm/i915/i915_params.c:173:
+#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)
-:53: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'name' - possible side-effects?
#53: FILE: drivers/gpu/drm/i915/i915_params.c:173:
+#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)
-:53: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'x' - possible side-effects?
#53: FILE: drivers/gpu/drm/i915/i915_params.c:173:
+#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)
total: 0 errors, 0 warnings, 3 checks, 35 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Convert _print_param to a macro
2018-10-09 17:14 [PATCH] drm/i915: Convert _print_param to a macro Nathan Chancellor
2018-10-09 20:59 ` Nick Desaulniers
2018-10-10 9:34 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
@ 2018-10-10 9:50 ` Patchwork
2018-10-10 11:23 ` ✓ Fi.CI.IGT: " Patchwork
` (3 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2018-10-10 9:50 UTC (permalink / raw)
To: Nathan Chancellor; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Convert _print_param to a macro
URL : https://patchwork.freedesktop.org/series/50789/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4958 -> Patchwork_10408 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/50789/revisions/1/mbox/
== Known issues ==
Here are the changes found in Patchwork_10408 that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@kms_flip@basic-flip-vs-wf_vblank:
fi-bsw-kefka: PASS -> FAIL (fdo#100368)
igt@kms_pipe_crc_basic@read-crc-pipe-a:
fi-byt-clapper: PASS -> FAIL (fdo#107362)
==== Possible fixes ====
igt@kms_frontbuffer_tracking@basic:
fi-byt-clapper: FAIL (fdo#103167) -> PASS
igt@pm_rpm@basic-pci-d3-state:
fi-skl-6600u: FAIL (fdo#107707) -> PASS
igt@prime_vgem@basic-fence-flip:
fi-ilk-650: FAIL (fdo#104008) -> PASS
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
fdo#107707 https://bugs.freedesktop.org/show_bug.cgi?id=107707
== Participating hosts (48 -> 41) ==
Missing (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-icl-u2 fi-bsw-cyan fi-ctg-p8600 fi-pnv-d510
== Build changes ==
* Linux: CI_DRM_4958 -> Patchwork_10408
CI_DRM_4958: 9990e1665029dc2ef4a9c0632b8a2f516263e595 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4672: 4497591d2572831a9f07fd9e48a2571bfcffe354 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10408: fb0e5c359f1a3d3ec2bfc1c08549f31c9976b801 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
fb0e5c359f1a drm/i915: Convert _print_param to a macro
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10408/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915: Convert _print_param to a macro
2018-10-09 17:14 [PATCH] drm/i915: Convert _print_param to a macro Nathan Chancellor
` (2 preceding siblings ...)
2018-10-10 9:50 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-10-10 11:23 ` Patchwork
2018-10-11 6:43 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Convert _print_param to a macro (rev2) Patchwork
` (2 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2018-10-10 11:23 UTC (permalink / raw)
To: Nathan Chancellor; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Convert _print_param to a macro
URL : https://patchwork.freedesktop.org/series/50789/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4958_full -> Patchwork_10408_full =
== Summary - WARNING ==
Minor unknown changes coming with Patchwork_10408_full need to be verified
manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_10408_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
== Possible new issues ==
Here are the unknown changes that may have been introduced in Patchwork_10408_full:
=== IGT changes ===
==== Warnings ====
igt@pm_rc6_residency@rc6-accuracy:
shard-kbl: SKIP -> PASS
shard-snb: SKIP -> PASS +1
== Known issues ==
Here are the changes found in Patchwork_10408_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@drv_suspend@shrink:
shard-skl: PASS -> INCOMPLETE (fdo#106886)
igt@gem_ctx_isolation@vcs0-s3:
shard-skl: PASS -> INCOMPLETE (fdo#104108, fdo#107773)
igt@gem_exec_schedule@pi-ringfull-vebox:
shard-skl: NOTRUN -> FAIL (fdo#103158)
igt@kms_busy@extended-modeset-hang-newfb-render-b:
shard-skl: NOTRUN -> DMESG-WARN (fdo#107956)
igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-c:
shard-hsw: NOTRUN -> DMESG-WARN (fdo#107956)
igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
shard-skl: NOTRUN -> FAIL (fdo#105458)
igt@kms_cursor_crc@cursor-256x85-random:
shard-apl: PASS -> FAIL (fdo#103232) +1
igt@kms_cursor_crc@cursor-64x64-random:
shard-skl: NOTRUN -> FAIL (fdo#103232)
igt@kms_cursor_legacy@cursorb-vs-flipb-toggle:
shard-glk: PASS -> DMESG-WARN (fdo#105763, fdo#106538) +1
igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-cpu:
shard-apl: PASS -> FAIL (fdo#103167) +1
igt@kms_frontbuffer_tracking@fbc-2p-primscrn-shrfb-pgflip-blt:
shard-glk: PASS -> FAIL (fdo#103167) +2
igt@kms_frontbuffer_tracking@fbc-stridechange:
shard-skl: NOTRUN -> FAIL (fdo#105683)
igt@kms_plane@pixel-format-pipe-c-planes:
shard-skl: NOTRUN -> DMESG-FAIL (fdo#106885, fdo#103166)
{igt@kms_plane_alpha_blend@pipe-c-alpha-7efc}:
shard-skl: NOTRUN -> FAIL (fdo#108146)
{igt@kms_plane_alpha_blend@pipe-c-alpha-transparant-fb}:
shard-skl: NOTRUN -> FAIL (fdo#108145) +1
igt@kms_plane_multiple@atomic-pipe-a-tiling-yf:
shard-apl: PASS -> FAIL (fdo#103166)
igt@kms_rotation_crc@exhaust-fences:
shard-skl: NOTRUN -> DMESG-WARN (fdo#105748)
igt@pm_rpm@debugfs-forcewake-user:
shard-skl: PASS -> INCOMPLETE (fdo#107807)
==== Possible fixes ====
igt@kms_busy@extended-modeset-hang-newfb-render-c:
shard-kbl: DMESG-WARN (fdo#107956) -> PASS
igt@kms_cursor_crc@cursor-128x42-onscreen:
shard-apl: FAIL (fdo#103232) -> PASS
igt@kms_cursor_crc@cursor-128x42-random:
shard-hsw: INCOMPLETE (fdo#103540) -> PASS
igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-blt:
shard-apl: INCOMPLETE (fdo#103927) -> PASS
igt@kms_plane@plane-position-covered-pipe-b-planes:
shard-glk: FAIL (fdo#103166) -> PASS +3
igt@kms_plane_multiple@atomic-pipe-b-tiling-x:
shard-apl: FAIL (fdo#103166) -> PASS +1
igt@perf@polling:
shard-hsw: FAIL (fdo#102252) -> PASS
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
fdo#105458 https://bugs.freedesktop.org/show_bug.cgi?id=105458
fdo#105683 https://bugs.freedesktop.org/show_bug.cgi?id=105683
fdo#105748 https://bugs.freedesktop.org/show_bug.cgi?id=105748
fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
fdo#106885 https://bugs.freedesktop.org/show_bug.cgi?id=106885
fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
fdo#108146 https://bugs.freedesktop.org/show_bug.cgi?id=108146
== Participating hosts (6 -> 6) ==
No changes in participating hosts
== Build changes ==
* Linux: CI_DRM_4958 -> Patchwork_10408
CI_DRM_4958: 9990e1665029dc2ef4a9c0632b8a2f516263e595 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4672: 4497591d2572831a9f07fd9e48a2571bfcffe354 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10408: fb0e5c359f1a3d3ec2bfc1c08549f31c9976b801 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10408/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Convert _print_param to a macro
2018-10-09 20:59 ` Nick Desaulniers
@ 2018-10-10 12:01 ` Jani Nikula
0 siblings, 0 replies; 15+ messages in thread
From: Jani Nikula @ 2018-10-10 12:01 UTC (permalink / raw)
To: Nick Desaulniers, Nathan Chancellor
Cc: joonas.lahtinen, rodrigo.vivi, intel-gfx, dri-devel, LKML, lukas.bulwahn
On Tue, 09 Oct 2018, Nick Desaulniers <ndesaulniers@google.com> wrote:
> 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).
*Sad trombone*
I'd rather see us converting more macros to static inlines than the
other way round.
I'll let others chime in if they have any better ideas, otherwise I'll
apply this one.
BR,
Jani.
>
>>
>> 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
>>
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Convert _print_param to a macro
@ 2018-10-10 12:01 ` Jani Nikula
0 siblings, 0 replies; 15+ messages in thread
From: Jani Nikula @ 2018-10-10 12:01 UTC (permalink / raw)
To: Nick Desaulniers, Nathan Chancellor
Cc: intel-gfx, LKML, dri-devel, rodrigo.vivi, lukas.bulwahn
On Tue, 09 Oct 2018, Nick Desaulniers <ndesaulniers@google.com> wrote:
> 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).
*Sad trombone*
I'd rather see us converting more macros to static inlines than the
other way round.
I'll let others chime in if they have any better ideas, otherwise I'll
apply this one.
BR,
Jani.
>
>>
>> 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
>>
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Convert _print_param to a macro
2018-10-10 12:01 ` Jani Nikula
(?)
@ 2018-10-10 20:30 ` Michal Wajdeczko
2018-10-10 20:41 ` Nick Desaulniers
-1 siblings, 1 reply; 15+ messages in thread
From: Michal Wajdeczko @ 2018-10-10 20:30 UTC (permalink / raw)
To: Nick Desaulniers, Nathan Chancellor, Jani Nikula
Cc: intel-gfx, LKML, dri-devel, rodrigo.vivi, lukas.bulwahn
On Wed, 10 Oct 2018 14:01:40 +0200, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Tue, 09 Oct 2018, Nick Desaulniers <ndesaulniers@google.com> wrote:
>> 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).
>
> *Sad trombone*
>
> I'd rather see us converting more macros to static inlines than the
> other way round.
>
> I'll let others chime in if they have any better ideas, otherwise I'll
> apply this one.
Option 1: Just drop BUILD_BUG() from _print_param() function.
Option 2: Use aliases instead of real types in param() macros.
Aliases can be same as in linux/moduleparam.h (charp|int|uint|bool)
We can convert aliases back to real types but it will also allow
to construct proper names for dedicated functions - see [1]
Michal
[1] https://patchwork.freedesktop.org/patch/255928/
>
> BR,
> Jani.
>
>>
>>>
>>> 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
>>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Convert _print_param to a macro
2018-10-10 20:30 ` [Intel-gfx] " Michal Wajdeczko
@ 2018-10-10 20:41 ` Nick Desaulniers
0 siblings, 0 replies; 15+ messages in thread
From: Nick Desaulniers @ 2018-10-10 20:41 UTC (permalink / raw)
To: michal.wajdeczko
Cc: Nathan Chancellor, jani.nikula, intel-gfx, LKML, dri-devel,
rodrigo.vivi, lukas.bulwahn
On Wed, Oct 10, 2018 at 1:30 PM Michal Wajdeczko
<michal.wajdeczko@intel.com> wrote:
>
> On Wed, 10 Oct 2018 14:01:40 +0200, Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
>
> > On Tue, 09 Oct 2018, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >> 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).
> >
> > *Sad trombone*
> >
> > I'd rather see us converting more macros to static inlines than the
> > other way round.
> >
> > I'll let others chime in if they have any better ideas, otherwise I'll
> > apply this one.
>
> Option 1: Just drop BUILD_BUG() from _print_param() function.
I was also thinking of this.
>
> Option 2: Use aliases instead of real types in param() macros.
Will that affect other users of I915_PARAMS_FOR_EACH than _print_param?
Either way, thanks for the help towards resolving this! We appreciate it!
>
> Aliases can be same as in linux/moduleparam.h (charp|int|uint|bool)
> We can convert aliases back to real types but it will also allow
> to construct proper names for dedicated functions - see [1]
>
> Michal
>
> [1] https://patchwork.freedesktop.org/patch/255928/
>
>
> >
> > BR,
> > Jani.
> >
> >>
> >>>
> >>> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Convert _print_param to a macro
@ 2018-10-10 20:41 ` Nick Desaulniers
0 siblings, 0 replies; 15+ messages in thread
From: Nick Desaulniers @ 2018-10-10 20:41 UTC (permalink / raw)
To: michal.wajdeczko
Cc: intel-gfx, LKML, dri-devel, rodrigo.vivi, lukas.bulwahn,
Nathan Chancellor
On Wed, Oct 10, 2018 at 1:30 PM Michal Wajdeczko
<michal.wajdeczko@intel.com> wrote:
>
> On Wed, 10 Oct 2018 14:01:40 +0200, Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
>
> > On Tue, 09 Oct 2018, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >> 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).
> >
> > *Sad trombone*
> >
> > I'd rather see us converting more macros to static inlines than the
> > other way round.
> >
> > I'll let others chime in if they have any better ideas, otherwise I'll
> > apply this one.
>
> Option 1: Just drop BUILD_BUG() from _print_param() function.
I was also thinking of this.
>
> Option 2: Use aliases instead of real types in param() macros.
Will that affect other users of I915_PARAMS_FOR_EACH than _print_param?
Either way, thanks for the help towards resolving this! We appreciate it!
>
> Aliases can be same as in linux/moduleparam.h (charp|int|uint|bool)
> We can convert aliases back to real types but it will also allow
> to construct proper names for dedicated functions - see [1]
>
> Michal
>
> [1] https://patchwork.freedesktop.org/patch/255928/
>
>
> >
> > BR,
> > Jani.
> >
> >>
> >>>
> >>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Convert _print_param to a macro
2018-10-10 20:41 ` Nick Desaulniers
(?)
@ 2018-10-11 6:17 ` Jani Nikula
2018-10-11 20:55 ` Nick Desaulniers
-1 siblings, 1 reply; 15+ messages in thread
From: Jani Nikula @ 2018-10-11 6:17 UTC (permalink / raw)
To: Nick Desaulniers, michal.wajdeczko
Cc: Nathan Chancellor, intel-gfx, LKML, dri-devel, rodrigo.vivi,
lukas.bulwahn
On Wed, 10 Oct 2018, Nick Desaulniers <ndesaulniers@google.com> wrote:
> On Wed, Oct 10, 2018 at 1:30 PM Michal Wajdeczko
> <michal.wajdeczko@intel.com> wrote:
>>
>> On Wed, 10 Oct 2018 14:01:40 +0200, Jani Nikula
>> <jani.nikula@linux.intel.com> wrote:
>>
>> > On Tue, 09 Oct 2018, Nick Desaulniers <ndesaulniers@google.com> wrote:
>> >> 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).
>> >
>> > *Sad trombone*
>> >
>> > I'd rather see us converting more macros to static inlines than the
>> > other way round.
>> >
>> > I'll let others chime in if they have any better ideas, otherwise I'll
>> > apply this one.
>>
>> Option 1: Just drop BUILD_BUG() from _print_param() function.
>
> I was also thinking of this.
So does this fix the issue?
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index bd6bd8879cab..8d71886b5f03 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -184,7 +184,8 @@ static __always_inline void _print_param(struct drm_printer *p,
else if (!__builtin_strcmp(type, "char *"))
drm_printf(p, "i915.%s=%s\n", name, *(const char **)x);
else
- BUILD_BUG();
+ WARN_ONCE(1, "no printer defined for param type %s (i915.%s)\n",
+ type, name);
}
/**
---
>
>>
>> Option 2: Use aliases instead of real types in param() macros.
>
> Will that affect other users of I915_PARAMS_FOR_EACH than _print_param?
>
> Either way, thanks for the help towards resolving this! We appreciate it!
>
>>
>> Aliases can be same as in linux/moduleparam.h (charp|int|uint|bool)
>> We can convert aliases back to real types but it will also allow
>> to construct proper names for dedicated functions - see [1]
>>
>> Michal
>>
>> [1] https://patchwork.freedesktop.org/patch/255928/
I can't find this on the list; was this sent just to patchwork or what?
BR,
Jani.
>>
>>
>> >
>> > BR,
>> > Jani.
>> >
>> >>
>> >>>
>> >>> 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
>> >>>
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply related [flat|nested] 15+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Convert _print_param to a macro (rev2)
2018-10-09 17:14 [PATCH] drm/i915: Convert _print_param to a macro Nathan Chancellor
` (3 preceding siblings ...)
2018-10-10 11:23 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-10-11 6:43 ` Patchwork
2018-10-11 6:59 ` ✓ Fi.CI.BAT: success " Patchwork
2018-10-11 13:47 ` ✓ Fi.CI.IGT: " Patchwork
6 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2018-10-11 6:43 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Convert _print_param to a macro (rev2)
URL : https://patchwork.freedesktop.org/series/50789/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
a67cd1f0b8b2 drm/i915: Convert _print_param to a macro
-:21: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#21:
>> >>> ld: drivers/gpu/drm/i915/i915_params.o: in function `i915_params_dump':
-:74: ERROR:MISSING_SIGN_OFF: Missing Signed-off-by: line(s)
total: 1 errors, 1 warnings, 0 checks, 9 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Convert _print_param to a macro (rev2)
2018-10-09 17:14 [PATCH] drm/i915: Convert _print_param to a macro Nathan Chancellor
` (4 preceding siblings ...)
2018-10-11 6:43 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Convert _print_param to a macro (rev2) Patchwork
@ 2018-10-11 6:59 ` Patchwork
2018-10-11 13:47 ` ✓ Fi.CI.IGT: " Patchwork
6 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2018-10-11 6:59 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Convert _print_param to a macro (rev2)
URL : https://patchwork.freedesktop.org/series/50789/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4968 -> Patchwork_10422 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/50789/revisions/2/mbox/
== Known issues ==
Here are the changes found in Patchwork_10422 that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@amdgpu/amd_basic@cs-compute:
fi-kbl-8809g: NOTRUN -> FAIL (fdo#108094)
igt@amdgpu/amd_prime@amd-to-i915:
fi-kbl-8809g: NOTRUN -> FAIL (fdo#107341)
igt@kms_frontbuffer_tracking@basic:
fi-byt-clapper: PASS -> FAIL (fdo#103167)
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
fi-byt-clapper: PASS -> FAIL (fdo#107362, fdo#103191)
==== Possible fixes ====
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
fi-cfl-8109u: INCOMPLETE (fdo#106070, fdo#108126) -> PASS
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#106070 https://bugs.freedesktop.org/show_bug.cgi?id=106070
fdo#107341 https://bugs.freedesktop.org/show_bug.cgi?id=107341
fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
fdo#108094 https://bugs.freedesktop.org/show_bug.cgi?id=108094
fdo#108126 https://bugs.freedesktop.org/show_bug.cgi?id=108126
== Participating hosts (45 -> 40) ==
Additional (1): fi-pnv-d510
Missing (6): fi-ilk-m540 fi-byt-squawks fi-icl-u2 fi-bsw-cyan fi-ctg-p8600 fi-byt-n2820
== Build changes ==
* Linux: CI_DRM_4968 -> Patchwork_10422
CI_DRM_4968: 6c3870cc045403bc59ab66ea6ca4bad98114d0a4 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4673: 54cb1aeb4e50dea9f3abae632e317875d147c4ab @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10422: a67cd1f0b8b2d1d163f1856451cd98f3dc76440a @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
a67cd1f0b8b2 drm/i915: Convert _print_param to a macro
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10422/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915: Convert _print_param to a macro (rev2)
2018-10-09 17:14 [PATCH] drm/i915: Convert _print_param to a macro Nathan Chancellor
` (5 preceding siblings ...)
2018-10-11 6:59 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-10-11 13:47 ` Patchwork
6 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2018-10-11 13:47 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Convert _print_param to a macro (rev2)
URL : https://patchwork.freedesktop.org/series/50789/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4968_full -> Patchwork_10422_full =
== Summary - SUCCESS ==
No regressions found.
== Known issues ==
Here are the changes found in Patchwork_10422_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@gem_exec_schedule@pi-ringfull-bsd:
shard-skl: NOTRUN -> FAIL (fdo#103158) +2
igt@gem_mmap@bad-object:
shard-apl: PASS -> DMESG-WARN (fdo#105602, fdo#103558)
igt@kms_addfb_basic@bo-too-small-due-to-tiling:
shard-snb: NOTRUN -> DMESG-WARN (fdo#107469) +1
igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
shard-skl: NOTRUN -> DMESG-WARN (fdo#107956)
igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
shard-snb: NOTRUN -> DMESG-WARN (fdo#107956) +1
igt@kms_cursor_legacy@cursorb-vs-flipb-toggle:
shard-glk: PASS -> DMESG-WARN (fdo#106538, fdo#105763)
igt@kms_cursor_legacy@pipe-c-forked-move:
shard-apl: PASS -> INCOMPLETE (fdo#103927)
igt@kms_flip@flip-vs-expired-vblank:
shard-glk: PASS -> FAIL (fdo#105363, fdo#102887)
igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt:
shard-glk: PASS -> FAIL (fdo#103167)
igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-indfb-draw-mmap-cpu:
shard-glk: PASS -> DMESG-FAIL (fdo#106538)
igt@kms_frontbuffer_tracking@fbcpsr-suspend:
shard-skl: PASS -> INCOMPLETE (fdo#107773, fdo#104108)
igt@kms_pipe_crc_basic@read-crc-pipe-c:
shard-skl: NOTRUN -> FAIL (fdo#107362) +1
igt@kms_plane@pixel-format-pipe-c-planes:
shard-skl: NOTRUN -> DMESG-FAIL (fdo#103166, fdo#106885)
{igt@kms_plane_alpha_blend@pipe-a-alpha-7efc}:
shard-skl: NOTRUN -> FAIL (fdo#108145) +3
{igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb}:
shard-kbl: NOTRUN -> FAIL (fdo#108145)
{igt@kms_plane_alpha_blend@pipe-c-alpha-7efc}:
shard-skl: NOTRUN -> FAIL (fdo#108146)
{igt@kms_plane_alpha_blend@pipe-c-coverage-7efc}:
shard-apl: NOTRUN -> FAIL (fdo#108146)
igt@kms_plane_multiple@atomic-pipe-b-tiling-x:
shard-skl: NOTRUN -> FAIL (fdo#103166)
igt@kms_setmode@basic:
shard-kbl: PASS -> FAIL (fdo#99912)
igt@kms_sysfs_edid_timing:
shard-skl: NOTRUN -> FAIL (fdo#100047)
igt@kms_vblank@pipe-b-wait-forked:
shard-snb: NOTRUN -> INCOMPLETE (fdo#105411)
igt@perf@blocking:
shard-hsw: PASS -> FAIL (fdo#102252)
igt@perf_pmu@rc6-runtime-pm:
shard-glk: PASS -> FAIL (fdo#105010)
igt@pm_rpm@sysfs-read:
shard-skl: PASS -> INCOMPLETE (fdo#107807)
==== Possible fixes ====
igt@drv_hangman@error-state-capture-render:
shard-glk: INCOMPLETE (fdo#103359, k.org#198133) -> PASS
igt@gem_userptr_blits@readonly-unsync:
shard-kbl: INCOMPLETE (fdo#103665) -> PASS
igt@kms_busy@extended-pageflip-hang-newfb-render-a:
shard-glk: DMESG-WARN (fdo#107956) -> PASS
igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
shard-kbl: DMESG-WARN (fdo#107956) -> PASS
igt@kms_color@pipe-a-ctm-max:
shard-skl: FAIL (fdo#108147) -> PASS
igt@kms_cursor_crc@cursor-256x256-random:
shard-apl: FAIL (fdo#103232) -> PASS +2
igt@kms_flip@flip-vs-expired-vblank:
shard-kbl: FAIL (fdo#105363, fdo#102887) -> PASS
igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt:
shard-apl: FAIL (fdo#103167) -> PASS +2
igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-pwrite:
shard-glk: FAIL (fdo#103167) -> PASS +1
igt@kms_frontbuffer_tracking@psr-farfromfence:
shard-skl: FAIL (fdo#103167) -> PASS
igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes:
shard-skl: INCOMPLETE (fdo#107773, fdo#104108) -> PASS +1
{igt@kms_plane_alpha_blend@pipe-a-coverage-7efc}:
shard-skl: FAIL (fdo#108145) -> PASS
igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
shard-glk: FAIL (fdo#103166) -> PASS
igt@kms_plane_multiple@atomic-pipe-c-tiling-yf:
shard-apl: FAIL (fdo#103166) -> PASS
igt@perf_pmu@rc6-runtime-pm:
shard-apl: FAIL (fdo#105010) -> PASS
==== Warnings ====
igt@kms_busy@extended-modeset-hang-newfb-render-b:
shard-apl: INCOMPLETE (fdo#103927) -> DMESG-WARN (fdo#107956)
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
fdo#105010 https://bugs.freedesktop.org/show_bug.cgi?id=105010
fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
fdo#106885 https://bugs.freedesktop.org/show_bug.cgi?id=106885
fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
fdo#107469 https://bugs.freedesktop.org/show_bug.cgi?id=107469
fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
fdo#108146 https://bugs.freedesktop.org/show_bug.cgi?id=108146
fdo#108147 https://bugs.freedesktop.org/show_bug.cgi?id=108147
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133
== Participating hosts (6 -> 6) ==
No changes in participating hosts
== Build changes ==
* Linux: CI_DRM_4968 -> Patchwork_10422
CI_DRM_4968: 6c3870cc045403bc59ab66ea6ca4bad98114d0a4 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4673: 54cb1aeb4e50dea9f3abae632e317875d147c4ab @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10422: a67cd1f0b8b2d1d163f1856451cd98f3dc76440a @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10422/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Convert _print_param to a macro
2018-10-11 6:17 ` [Intel-gfx] " Jani Nikula
@ 2018-10-11 20:55 ` Nick Desaulniers
0 siblings, 0 replies; 15+ messages in thread
From: Nick Desaulniers @ 2018-10-11 20:55 UTC (permalink / raw)
To: jani.nikula
Cc: michal.wajdeczko, Nathan Chancellor, intel-gfx, LKML, dri-devel,
rodrigo.vivi, lukas.bulwahn
On Wed, Oct 10, 2018 at 11:21 PM Jani Nikula
<jani.nikula@linux.intel.com> wrote:
>
> On Wed, 10 Oct 2018, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > On Wed, Oct 10, 2018 at 1:30 PM Michal Wajdeczko
> > <michal.wajdeczko@intel.com> wrote:
> >>
> >> On Wed, 10 Oct 2018 14:01:40 +0200, Jani Nikula
> >> <jani.nikula@linux.intel.com> wrote:
> >>
> >> > On Tue, 09 Oct 2018, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >> >> 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).
> >> >
> >> > *Sad trombone*
> >> >
> >> > I'd rather see us converting more macros to static inlines than the
> >> > other way round.
> >> >
> >> > I'll let others chime in if they have any better ideas, otherwise I'll
> >> > apply this one.
> >>
> >> Option 1: Just drop BUILD_BUG() from _print_param() function.
> >
> > I was also thinking of this.
>
> So does this fix the issue?
Yes, that should do the trick. I assume this macro can also be
rewritten to use __builtin_types_compatible_p and
__builtin_choose_expr (rather than tokenizing the type and using
__builtin_strcmp), but maybe an exercise for another day. We're happy
with the simplest fix acceptable for now.
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index bd6bd8879cab..8d71886b5f03 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -184,7 +184,8 @@ static __always_inline void _print_param(struct drm_printer *p,
> else if (!__builtin_strcmp(type, "char *"))
> drm_printf(p, "i915.%s=%s\n", name, *(const char **)x);
> else
> - BUILD_BUG();
> + WARN_ONCE(1, "no printer defined for param type %s (i915.%s)\n",
> + type, name);
> }
>
> /**
>
> ---
>
> >
> >>
> >> Option 2: Use aliases instead of real types in param() macros.
> >
> > Will that affect other users of I915_PARAMS_FOR_EACH than _print_param?
> >
> > Either way, thanks for the help towards resolving this! We appreciate it!
> >
> >>
> >> Aliases can be same as in linux/moduleparam.h (charp|int|uint|bool)
> >> We can convert aliases back to real types but it will also allow
> >> to construct proper names for dedicated functions - see [1]
> >>
> >> Michal
> >>
> >> [1] https://patchwork.freedesktop.org/patch/255928/
>
> I can't find this on the list; was this sent just to patchwork or what?
>
> BR,
> Jani.
>
>
> >>
> >>
> >> >
> >> > BR,
> >> > Jani.
> >> >
> >> >>
> >> >>>
> >> >>> 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
> >> >>>
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-10-11 20:55 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-09 17:14 [PATCH] drm/i915: Convert _print_param to a macro Nathan Chancellor
2018-10-09 20:59 ` Nick Desaulniers
2018-10-10 12:01 ` Jani Nikula
2018-10-10 12:01 ` Jani Nikula
2018-10-10 20:30 ` [Intel-gfx] " Michal Wajdeczko
2018-10-10 20:41 ` Nick Desaulniers
2018-10-10 20:41 ` Nick Desaulniers
2018-10-11 6:17 ` [Intel-gfx] " Jani Nikula
2018-10-11 20:55 ` Nick Desaulniers
2018-10-10 9:34 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2018-10-10 9:50 ` ✓ Fi.CI.BAT: success " Patchwork
2018-10-10 11:23 ` ✓ Fi.CI.IGT: " Patchwork
2018-10-11 6:43 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Convert _print_param to a macro (rev2) Patchwork
2018-10-11 6:59 ` ✓ Fi.CI.BAT: success " Patchwork
2018-10-11 13:47 ` ✓ Fi.CI.IGT: " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.