drm/i915 target adds some extra cflags, especially it does re-apply -Wall. In clang this will override -Wno-format-security and cause the build to fail when CONFIG_DRM_I915_WERROR=y. While with GCC this does not happen. We reapply -Wno-format-security here to get around this issue.
drivers/gpu/drm/i915/gt/intel_gt.c:983:2: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] GEM_TRACE("ERROR\n"); ^~~~~~~~~~~~~~~~~~~~ ./drivers/gpu/drm/i915/i915_gem.h:76:24: note: expanded from macro 'GEM_TRACE' #define GEM_TRACE(...) trace_printk(__VA_ARGS__) ^~~~~~~~~~~~~~~~~~~~~~~~~ ./include/linux/kernel.h:369:3: note: expanded from macro 'trace_printk' do_trace_printk(fmt, ##__VA_ARGS__); \ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./include/linux/kernel.h:383:30: note: expanded from macro 'do_trace_printk' __trace_bprintk(_THIS_IP_, trace_printk_fmt, ##args); \ ^~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/intel_gt.c:983:2: note: treat the string as an argument to avoid this
Signed-off-by: Tong Zhang ztong0001@gmail.com --- drivers/gpu/drm/i915/Makefile | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 1b62b9f65196..c04e05a3d39f 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -13,6 +13,7 @@ # will most likely get a sudden build breakage... Hopefully we will fix # new warnings before CI updates! subdir-ccflags-y := -Wall -Wextra +subdir-ccflags-y += -Wno-format-security subdir-ccflags-y += -Wno-unused-parameter subdir-ccflags-y += -Wno-type-limits subdir-ccflags-y += -Wno-missing-field-initializers
On Sat, Feb 12, 2022 at 10:51:06PM -0800, Tong Zhang wrote:
drm/i915 target adds some extra cflags, especially it does re-apply -Wall. In clang this will override -Wno-format-security and cause the build to fail when CONFIG_DRM_I915_WERROR=y. While with GCC this does not happen. We reapply -Wno-format-security here to get around this issue.
For what it's worth, GCC would warn in the exact same way if -Wformat-security was included within -Wall like it is for clang:
drivers/gpu/drm/i915/gt/intel_gt.c: In function ‘intel_gt_invalidate_tlbs’: drivers/gpu/drm/i915/gt/intel_gt.c:988:9: error: format not a string literal and no format arguments [-Werror=format-security] 988 | GEM_TRACE("\n"); | ^~~~~~~~~ cc1: all warnings being treated as errors
drivers/gpu/drm/i915/gt/selftest_execlists.c: In function ‘live_sanitycheck’: drivers/gpu/drm/i915/gt/selftest_execlists.c:142:25: error: format not a string literal and no format arguments [-Werror=format-security] 142 | GEM_TRACE("spinner failed to start\n"); | ^~~~~~~~~ drivers/gpu/drm/i915/gt/selftest_execlists.c: In function ‘live_preempt’: drivers/gpu/drm/i915/gt/selftest_execlists.c:1775:25: error: format not a string literal and no format arguments [-Werror=format-security] 1775 | GEM_TRACE("lo spinner failed to start\n"); | ^~~~~~~~~ drivers/gpu/drm/i915/gt/selftest_execlists.c:1792:25: error: format not a string literal and no format arguments [-Werror=format-security] 1792 | GEM_TRACE("hi spinner failed to start\n"); | ^~~~~~~~~ cc1: all warnings being treated as errors
If fixing these warnings instead of just disabling the warning is undesirable (since I feel like the entire point of the i195 cflags situation is to enable more warnings than the main Makefile), I think the commit message should be rewritten to something along the lines of:
"drm/i915 adds some extra cflags, namely -Wall, which causes instances of -Wformat-security to appear when building with clang, even though this warning is turned off kernel-wide in the main Makefile:"
drivers/gpu/drm/i915/gt/intel_gt.c:983:2: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] GEM_TRACE("ERROR\n"); ^~~~~~~~~~~~~~~~~~~~ ./drivers/gpu/drm/i915/i915_gem.h:76:24: note: expanded from macro 'GEM_TRACE' #define GEM_TRACE(...) trace_printk(__VA_ARGS__) ^~~~~~~~~~~~~~~~~~~~~~~~~ ./include/linux/kernel.h:369:3: note: expanded from macro 'trace_printk' do_trace_printk(fmt, ##__VA_ARGS__); \ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./include/linux/kernel.h:383:30: note: expanded from macro 'do_trace_printk' __trace_bprintk(_THIS_IP_, trace_printk_fmt, ##args); \ ^~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/intel_gt.c:983:2: note: treat the string as an argument to avoid this
"This does not happen with GCC because it does not enable -Wformat-security with -Wall. Disable -Wformat-security within the i915 Makefile so that these warnings do not show up with clang."
The actual diff itself looks fine to me.
Cheers, Nathan
Signed-off-by: Tong Zhang ztong0001@gmail.com
drivers/gpu/drm/i915/Makefile | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 1b62b9f65196..c04e05a3d39f 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -13,6 +13,7 @@ # will most likely get a sudden build breakage... Hopefully we will fix # new warnings before CI updates! subdir-ccflags-y := -Wall -Wextra +subdir-ccflags-y += -Wno-format-security subdir-ccflags-y += -Wno-unused-parameter subdir-ccflags-y += -Wno-type-limits subdir-ccflags-y += -Wno-missing-field-initializers -- 2.25.1
On Sun, Feb 13, 2022 at 10:39 AM Nathan Chancellor nathan@kernel.org wrote:
On Sat, Feb 12, 2022 at 10:51:06PM -0800, Tong Zhang wrote:
drm/i915 target adds some extra cflags, especially it does re-apply -Wall. In clang this will override -Wno-format-security and cause the build to fail when CONFIG_DRM_I915_WERROR=y. While with GCC this does not happen. We reapply -Wno-format-security here to get around this issue.
For what it's worth, GCC would warn in the exact same way if -Wformat-security was included within -Wall like it is for clang:
drivers/gpu/drm/i915/gt/intel_gt.c: In function ‘intel_gt_invalidate_tlbs’: drivers/gpu/drm/i915/gt/intel_gt.c:988:9: error: format not a string literal and no format arguments [-Werror=format-security] 988 | GEM_TRACE("\n"); | ^~~~~~~~~ cc1: all warnings being treated as errors
drivers/gpu/drm/i915/gt/selftest_execlists.c: In function ‘live_sanitycheck’: drivers/gpu/drm/i915/gt/selftest_execlists.c:142:25: error: format not a string literal and no format arguments [-Werror=format-security] 142 | GEM_TRACE("spinner failed to start\n"); | ^~~~~~~~~ drivers/gpu/drm/i915/gt/selftest_execlists.c: In function ‘live_preempt’: drivers/gpu/drm/i915/gt/selftest_execlists.c:1775:25: error: format not a string literal and no format arguments [-Werror=format-security] 1775 | GEM_TRACE("lo spinner failed to start\n"); | ^~~~~~~~~ drivers/gpu/drm/i915/gt/selftest_execlists.c:1792:25: error: format not a string literal and no format arguments [-Werror=format-security] 1792 | GEM_TRACE("hi spinner failed to start\n"); | ^~~~~~~~~ cc1: all warnings being treated as errors
If fixing these warnings instead of just disabling the warning is undesirable (since I feel like the entire point of the i195 cflags situation is to enable more warnings than the main Makefile), I think the commit message should be rewritten to something along the lines of:
"drm/i915 adds some extra cflags, namely -Wall, which causes instances of -Wformat-security to appear when building with clang, even though this warning is turned off kernel-wide in the main Makefile:"
drivers/gpu/drm/i915/gt/intel_gt.c:983:2: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] GEM_TRACE("ERROR\n"); ^~~~~~~~~~~~~~~~~~~~ ./drivers/gpu/drm/i915/i915_gem.h:76:24: note: expanded from macro 'GEM_TRACE' #define GEM_TRACE(...) trace_printk(__VA_ARGS__) ^~~~~~~~~~~~~~~~~~~~~~~~~ ./include/linux/kernel.h:369:3: note: expanded from macro 'trace_printk' do_trace_printk(fmt, ##__VA_ARGS__); \ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./include/linux/kernel.h:383:30: note: expanded from macro 'do_trace_printk' __trace_bprintk(_THIS_IP_, trace_printk_fmt, ##args); \ ^~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/intel_gt.c:983:2: note: treat the string as an argument to avoid this
"This does not happen with GCC because it does not enable -Wformat-security with -Wall. Disable -Wformat-security within the i915 Makefile so that these warnings do not show up with clang."
The actual diff itself looks fine to me.
Cheers, Nathan
Signed-off-by: Tong Zhang ztong0001@gmail.com
drivers/gpu/drm/i915/Makefile | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 1b62b9f65196..c04e05a3d39f 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -13,6 +13,7 @@ # will most likely get a sudden build breakage... Hopefully we will fix # new warnings before CI updates! subdir-ccflags-y := -Wall -Wextra +subdir-ccflags-y += -Wno-format-security subdir-ccflags-y += -Wno-unused-parameter subdir-ccflags-y += -Wno-type-limits subdir-ccflags-y += -Wno-missing-field-initializers -- 2.25.1
Thank you Nathan! I will send a v2 with a revised commit message. Thanks, - Tong
drm/i915 adds some extra cflags, namely -Wall, which causes instances of -Wformat-security to appear when building with clang, even though this warning is turned off kernel-wide in the main Makefile:
drivers/gpu/drm/i915/gt/intel_gt.c:983:2: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] GEM_TRACE("ERROR\n"); ^~~~~~~~~~~~~~~~~~~~ ./drivers/gpu/drm/i915/i915_gem.h:76:24: note: expanded from macro 'GEM_TRACE' #define GEM_TRACE(...) trace_printk(__VA_ARGS__) ^~~~~~~~~~~~~~~~~~~~~~~~~ ./include/linux/kernel.h:369:3: note: expanded from macro 'trace_printk' do_trace_printk(fmt, ##__VA_ARGS__); \ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./include/linux/kernel.h:383:30: note: expanded from macro 'do_trace_printk' __trace_bprintk(_THIS_IP_, trace_printk_fmt, ##args); \ ^~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/intel_gt.c:983:2: note: treat the string as an argument to avoid this
This does not happen with GCC because it does not enable -Wformat-security with -Wall. Disable -Wformat-security within the i915 Makefile so that these warnings do not show up with clang.
Signed-off-by: Tong Zhang ztong0001@gmail.com ---
v2: revise commit message
drivers/gpu/drm/i915/Makefile | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 1b62b9f65196..c04e05a3d39f 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -13,6 +13,7 @@ # will most likely get a sudden build breakage... Hopefully we will fix # new warnings before CI updates! subdir-ccflags-y := -Wall -Wextra +subdir-ccflags-y += -Wno-format-security subdir-ccflags-y += -Wno-unused-parameter subdir-ccflags-y += -Wno-type-limits subdir-ccflags-y += -Wno-missing-field-initializers
On Mon, Feb 14, 2022 at 11:58:20AM -0800, Tong Zhang wrote:
drm/i915 adds some extra cflags, namely -Wall, which causes instances of -Wformat-security to appear when building with clang, even though this warning is turned off kernel-wide in the main Makefile:
drivers/gpu/drm/i915/gt/intel_gt.c:983:2: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] GEM_TRACE("ERROR\n"); ^~~~~~~~~~~~~~~~~~~~ ./drivers/gpu/drm/i915/i915_gem.h:76:24: note: expanded from macro 'GEM_TRACE' #define GEM_TRACE(...) trace_printk(__VA_ARGS__) ^~~~~~~~~~~~~~~~~~~~~~~~~ ./include/linux/kernel.h:369:3: note: expanded from macro 'trace_printk' do_trace_printk(fmt, ##__VA_ARGS__); \ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./include/linux/kernel.h:383:30: note: expanded from macro 'do_trace_printk' __trace_bprintk(_THIS_IP_, trace_printk_fmt, ##args); \ ^~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/intel_gt.c:983:2: note: treat the string as an argument to avoid this
This does not happen with GCC because it does not enable -Wformat-security with -Wall. Disable -Wformat-security within the i915 Makefile so that these warnings do not show up with clang.
Signed-off-by: Tong Zhang ztong0001@gmail.com
Given this is not enabled for GCC and it is disabled in the main Makefile:
Reviewed-by: Nathan Chancellor nathan@kernel.org
Additionally, it seems like trace_printk() is designed to be able to take a string literal without a format argument, so this should be fine.
v2: revise commit message
drivers/gpu/drm/i915/Makefile | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 1b62b9f65196..c04e05a3d39f 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -13,6 +13,7 @@ # will most likely get a sudden build breakage... Hopefully we will fix # new warnings before CI updates! subdir-ccflags-y := -Wall -Wextra +subdir-ccflags-y += -Wno-format-security subdir-ccflags-y += -Wno-unused-parameter subdir-ccflags-y += -Wno-type-limits subdir-ccflags-y += -Wno-missing-field-initializers -- 2.25.1
On Tue, 15 Feb 2022, Nathan Chancellor nathan@kernel.org wrote:
On Mon, Feb 14, 2022 at 11:58:20AM -0800, Tong Zhang wrote:
drm/i915 adds some extra cflags, namely -Wall, which causes instances of -Wformat-security to appear when building with clang, even though this warning is turned off kernel-wide in the main Makefile:
drivers/gpu/drm/i915/gt/intel_gt.c:983:2: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] GEM_TRACE("ERROR\n"); ^~~~~~~~~~~~~~~~~~~~ ./drivers/gpu/drm/i915/i915_gem.h:76:24: note: expanded from macro 'GEM_TRACE' #define GEM_TRACE(...) trace_printk(__VA_ARGS__) ^~~~~~~~~~~~~~~~~~~~~~~~~ ./include/linux/kernel.h:369:3: note: expanded from macro 'trace_printk' do_trace_printk(fmt, ##__VA_ARGS__); \ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./include/linux/kernel.h:383:30: note: expanded from macro 'do_trace_printk' __trace_bprintk(_THIS_IP_, trace_printk_fmt, ##args); \ ^~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/intel_gt.c:983:2: note: treat the string as an argument to avoid this
This does not happen with GCC because it does not enable -Wformat-security with -Wall. Disable -Wformat-security within the i915 Makefile so that these warnings do not show up with clang.
Signed-off-by: Tong Zhang ztong0001@gmail.com
Given this is not enabled for GCC and it is disabled in the main Makefile:
Reviewed-by: Nathan Chancellor nathan@kernel.org
Additionally, it seems like trace_printk() is designed to be able to take a string literal without a format argument, so this should be fine.
Thanks for the patch and review, pushed to drm-intel-next. I appreciate the support in maintaining fairly strict warning levels in i915.
BR, Jani.
v2: revise commit message
drivers/gpu/drm/i915/Makefile | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 1b62b9f65196..c04e05a3d39f 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -13,6 +13,7 @@ # will most likely get a sudden build breakage... Hopefully we will fix # new warnings before CI updates! subdir-ccflags-y := -Wall -Wextra +subdir-ccflags-y += -Wno-format-security subdir-ccflags-y += -Wno-unused-parameter subdir-ccflags-y += -Wno-type-limits subdir-ccflags-y += -Wno-missing-field-initializers -- 2.25.1
dri-devel@lists.freedesktop.org
-
Jani Nikula -
Nathan Chancellor -
Tong Zhang