This snippet from the Linux kernel reads a data structure from an architecturally defined location in memory into a local copy: struct sharpsl_param_info { unsigned int comadj_keyword; }; extern struct sharpsl_param_info sharpsl_param; typedef unsigned long __kernel_size_t; extern void * memcpy(void *, const void *, __kernel_size_t); void sharpsl_save_param(void) { memcpy(&sharpsl_param, (void *)(0xe8ffc000), sizeof(struct sharpsl_param_info)); } With gcc-11, this now triggers a -Wstringop-overread warning on x86: arch/arm/common/sharpsl_param.i: In function ‘sharpsl_save_param’: arch/arm/common/sharpsl_param.i:11:2: warning: ‘memcpy’ reading 4 bytes from a region of size 0 [-Wstringop-overread] 11 | memcpy(&sharpsl_param, (void *)(0xe8ffc000), sizeof(struct sharpsl_param_info)); I tried to reproduce this on godbolt.org, which apparently has a slightly different snapshot version and instead produces -Warray-bounds warning for the same input: https://godbolt.org/z/ve6h6b I could not find a way to avoid this warning, other than turning off the entire warning option globally or with a pragma. Accessing a pointer from a literal integer value is not too unusual in the kernel and should not cause a warning.
The warning is by design. Its purpose is to detect invalid accesses at non-zero offsets to null pointers, like in the memset call below: struct S { int a, b[4]; }; void f (struct S *p) { if (p) return; memset (p->b, 0, 4 * sizeof p->b); } For now, I recommend suppressing the warning either by #pragma GCC diagnostic or by making the pointer volatile. In the future, providing an attribute to annotate constant addresses with (or extending the io() attribute supported by some targets to all targets) might be another way to avoid it.
Ok, I see. Thanks for the explanation! I found a couple other instances (so far all false positive) and will see if any of them have a sane workaround. I'll probably just turn off both flags globally for the kernel otherwise.
After some more analysis, I found that the -Wstringop-overread warning only happens here (and presumably in all the other cases I found) because I disabled -Warray-bounds for gcc-11. I'm still looking at -Warray-bounds to see what has changed here. There were some interesting findings from that one, but the number of added warnings made it hard to keep track of what is going on. It appears that the -Wstringop-overread warnings mostly a subset of those.
Most warnings designed to detect invalid accesses (not just -Wstringop-overread but also -Wstringop-overflow and -Wformat-overflow/-truncation, -Wrestrict, and some forms of -Warray-bounds) use the same underlying code to determine the identity of the accessed object, so they all should trigger if they see a constant address. But I tested the warning with the kernel when I implemented it months ago and don't think I saw any instances of it (though I don't see sharpsl_param in any of my logs). I still don't. How many do you see? Here's the list of -Wstringop- warnings in my fresh build but I'm never sure I use the right target. Is allyesconfig the right one? $ grep Wstringop-over /src/linux-stable/gcc-master.log arch/x86/mm/pgtable.c:437:13: warning: ‘preallocate_pmds.constprop’ accessing 8 bytes in a region of size 0 [-Wstringop-overflow=] arch/x86/mm/pgtable.c:440:13: warning: ‘preallocate_pmds.constprop’ accessing 8 bytes in a region of size 0 [-Wstringop-overflow=] arch/x86/mm/pgtable.c:462:9: warning: ‘free_pmds.constprop’ accessing 8 bytes in a region of size 0 [-Wstringop-overflow=] arch/x86/mm/pgtable.c:455:9: warning: ‘pgd_prepopulate_user_pmd’ accessing 8 bytes in a region of size 0 [-Wstringop-overflow=] arch/x86/mm/pgtable.c:464:9: warning: ‘free_pmds.constprop’ accessing 8 bytes in a region of size 0 [-Wstringop-overflow=] mm/mempolicy.c:3001:26: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=] mm/mempolicy.c:3001:26: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=] mm/mempolicy.c:3001:26: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=] mm/mempolicy.c:3001:26: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=] mm/mempolicy.c:3001:26: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=] mm/mempolicy.c:3001:26: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=] drivers/gpu/drm/i915/intel_pm.c:3093:9: warning: ‘intel_read_wm_latency’ accessing 16 bytes in a region of size 10 [-Wstringop-overflow=] drivers/gpu/drm/i915/intel_pm.c:3057:9: warning: ‘intel_print_wm_latency’ reading 16 bytes from a region of size 10 [-Wstringop-overread] drivers/gpu/drm/i915/intel_pm.c:3058:9: warning: ‘intel_print_wm_latency’ reading 16 bytes from a region of size 10 [-Wstringop-overread] drivers/gpu/drm/i915/intel_pm.c:3059:9: warning: ‘intel_print_wm_latency’ reading 16 bytes from a region of size 10 [-Wstringop-overread] drivers/gpu/drm/i915/intel_pm.c:3086:9: warning: ‘intel_print_wm_latency’ reading 16 bytes from a region of size 10 [-Wstringop-overread] drivers/gpu/drm/i915/intel_pm.c:3087:9: warning: ‘intel_print_wm_latency’ reading 16 bytes from a region of size 10 [-Wstringop-overread] drivers/gpu/drm/i915/intel_pm.c:3088:9: warning: ‘intel_print_wm_latency’ reading 16 bytes from a region of size 10 [-Wstringop-overread] drivers/gpu/drm/i915/intel_pm.c:3103:9: warning: ‘intel_print_wm_latency’ reading 16 bytes from a region of size 10 [-Wstringop-overread] drivers/gpu/drm/i915/intel_pm.c:3104:9: warning: ‘intel_print_wm_latency’ reading 16 bytes from a region of size 10 [-Wstringop-overread] drivers/gpu/drm/i915/intel_pm.c:3105:9: warning: ‘intel_print_wm_latency’ reading 16 bytes from a region of size 10 [-Wstringop-overread] drivers/gpu/drm/i915/display/intel_dp.c:4556:22: warning: ‘drm_dp_channel_eq_ok’ reading 6 bytes from a region of size 4 [-Wstringop-overread] The full breakdown with the warnings forcefully disabled in the top-level Makefile re-enabled is below: Diagnostic Count Unique Files -Wmissing-prototypes 759 248 114 -Wunused-const-variable= 391 233 31 -Wformat-truncation= 311 297 229 -Wmaybe-uninitialized 158 133 103 -Wunused-but-set-variable 143 137 88 -Warray-bounds 94 32 12 -Wzero-length-bounds 69 66 16 -Wsuggest-attribute=format 60 26 21 -Wnested-externs 41 1 1 -Woverride-init 36 22 15 -Wrestrict 23 14 10 -Wformat-overflow= 20 19 15 -Wempty-body 15 15 8 -Wstringop-overflow= 12 7 3 -Wmisleading-indentation 11 2 2 -Wcast-function-type 11 2 2 -Wstringop-overread 10 10 2 -Wenum-conversion 10 10 5 -Warray-parameter= 8 8 6 -Wpacked-not-aligned 5 3 2 -Wold-style-declaration 3 3 2 -Wignored-qualifiers 1 1 1 -Wconflicts-sr 1 1 1 -Wconflicts-rr 1 1 1
(In reply to Martin Sebor from comment #4) > Most warnings designed to detect invalid accesses (not just > -Wstringop-overread but also -Wstringop-overflow and > -Wformat-overflow/-truncation, -Wrestrict, and some forms of -Warray-bounds) > use the same underlying code to determine the identity of the accessed > object, so they all should trigger if they see a constant address. Right, makes sense. > But I tested the warning with the kernel when I implemented it months ago > and don't think I saw any instances of it (though I don't see sharpsl_param > in any of my logs). I still don't. How many do you see? > > Here's the list of -Wstringop- warnings in my fresh build but I'm never sure > I use the right target. Is allyesconfig the right one? For brief testing I usually test 'allmodconfig', which has slightly better coverage and is much faster to build than 'allyesconfig'. However, most of my testing is on random configurations, with a lot of patches on top to address all the known warnings. The sharpsl_param only shows up in unusual builds since this is a legacy Arm platform that needs a custom kernel configuration and is incompatible with normal armv5 builds. Some of these tend to only show up with certain combinations of the various sanitizers and inlining decisions such as -O2 vs -Os. > $ grep Wstringop-over /src/linux-stable/gcc-master.log > arch/x86/mm/pgtable.c:437:13: warning: ‘preallocate_pmds.constprop’ > accessing 8 bytes in a region of size 0 [-Wstringop-overflow=] > arch/x86/mm/pgtable.c:440:13: warning: ‘preallocate_pmds.constprop’ > accessing 8 bytes in a region of size 0 [-Wstringop-overflow=] > arch/x86/mm/pgtable.c:462:9: warning: ‘free_pmds.constprop’ accessing 8 > bytes in a region of size 0 [-Wstringop-overflow=] > arch/x86/mm/pgtable.c:455:9: warning: ‘pgd_prepopulate_user_pmd’ accessing 8 > bytes in a region of size 0 [-Wstringop-overflow=] > arch/x86/mm/pgtable.c:464:9: warning: ‘free_pmds.constprop’ accessing 8 > bytes in a region of size 0 [-Wstringop-overflow=] I don't see these at the moment, maybe the kernel already got fixed for them. > mm/mempolicy.c:3001:26: warning: writing 1 byte into a region of size 0 > [-Wstringop-overflow=] Nor this one. > drivers/gpu/drm/i915/intel_pm.c:3093:9: warning: ‘intel_read_wm_latency’ > accessing 16 bytes in a region of size 10 [-Wstringop-overflow=] > drivers/gpu/drm/i915/intel_pm.c:3057:9: warning: ‘intel_print_wm_latency’ > reading 16 bytes from a region of size 10 [-Wstringop-overread] This looks like a reasonable warning in principle, though I think the code is still correct. I have a patch for this. > drivers/gpu/drm/i915/display/intel_dp.c:4556:22: warning: > ‘drm_dp_channel_eq_ok’ reading 6 bytes from a region of size 4 > [-Wstringop-overread] Different bug, similar verdict: I have a patch to work around it, it seems reasonable to warn about it, but I think the code is correct. Here is one that got added in gcc-11 I just couldn't figure out: https://godbolt.org/z/sjjGc9 On this one I understand why gcc warns (pointer is compared to known constant address), but the code is correct and I don't know how to rework the code other than using #pragma to turn off the warning: In file included from arch/x86/boot/compressed/misc.c:18: In function ‘parse_elf’, inlined from ‘extract_kernel’ at arch/x86/boot/compressed/misc.c:442:2: arch/x86/boot/compressed/../string.h:15:23: error: ‘__builtin_memcpy’ reading 64 bytes from a region of size 0 [-Werror=stringop-overread] 15 | #define memcpy(d,s,l) __builtin_memcpy(d,s,l) | ^~~~~~~~~~~~~~~~~~~~~~~ arch/x86/boot/compressed/misc.c:283:9: note: in expansion of macro ‘memcpy’ 283 | memcpy(&ehdr, output, sizeof(ehdr)); | ^~~~~~ This one is correct code, but has a simple workaround that does not make the code any uglier: security/commoncap.c: In function ‘cap_inode_getsecurity’: security/commoncap.c:440:33: error: ‘memcpy’ reading 16 bytes from a region of size 0 [-Werror=stringop-overread] 440 | memcpy(&nscap->data, &cap->data, sizeof(__le32) * 2 * VFS_CAP_U32); | - if (ret < 0) + if (ret < 0 || !tmpbuf) I also got this one (with -Warray-bounds, but seems related) that looks like a real bug in the kernel: net/core/skbuff.c: In function ‘skb_find_text’: net/core/skbuff.c:3498:26: error: array subscript ‘struct skb_seq_state[0]’ is partly outside array bounds of ‘struct ts_state[1]’ [-Werror=array-bounds] I have a patch, but it needs to be discussed first. > The full breakdown with the warnings forcefully disabled in the top-level > Makefile re-enabled is below: > > Diagnostic Count Unique Files > -Wmissing-prototypes 759 248 114 > -Wunused-const-variable= 391 233 31 > -Wformat-truncation= 311 297 229 > -Wmaybe-uninitialized 158 133 103 > -Wunused-but-set-variable 143 137 88 > -Warray-bounds 94 32 12 > -Wzero-length-bounds 69 66 16 > -Wsuggest-attribute=format 60 26 21 > -Wnested-externs 41 1 1 > -Woverride-init 36 22 15 > -Wrestrict 23 14 10 > -Wformat-overflow= 20 19 15 > -Wempty-body 15 15 8 > -Wstringop-overflow= 12 7 3 > -Wmisleading-indentation 11 2 2 > -Wcast-function-type 11 2 2 > -Wstringop-overread 10 10 2 > -Wenum-conversion 10 10 5 > -Warray-parameter= 8 8 6 > -Wpacked-not-aligned 5 3 2 > -Wold-style-declaration 3 3 2 > -Wignored-qualifiers 1 1 1 > -Wconflicts-sr 1 1 1 > -Wconflicts-rr 1 1 1 Right, I see roughly the same, and I have patches for a lot of these. Lee Jones has sent several thousand patches over the past year to get to the point where we can almost turn most of these back on again. I particularly want to finally enable -Wmissing-prototypes, it seems we are getting fairly close to that (it used to be thousands of warnings). Linus Torvalds decided that Wmaybe-uninitialized was no longer worth enabling after the different inlining choices in (iirc) gcc-10 made the ratio of false-positive much worse than it was. I had spent a lot of time on addressing issues on that, but have pretty much given up on it as well. Fortunately, clang seems to be doing a reasonable job at finding the actual bugs here with -Wsometimes-uninitialized, so we still catch most of the important ones. Also, more kernels are now built with the -fplugin-arg-structleak_plugin-byref-all option that initializes all escaping local variables to zero, which effectively disables the Wmaybe-uninitialized output anyway but at least makes the behavior more deterministic. I have enabled -Wextra for my local builds now, which mostly works. The interesting options I still leave disabled are -Wpsabi, -Wshift-negative-value, -Waddress-of-packed-member, -Wframe-address, -Wstringop-truncation, -Wempty-body, -Wunused-but-set-variable, and -Wmissing-prototypes. For reference, here is the list of unique warnings I made a year ago: 5915026 -Wredundant-decls 4828168 -Wpacked 2241214 -Wswitch-enum 1266195 -Wsign-compare 1171275 -Winline 1141263 -Wlarger-than= 1075660 -Wmissing-field-initializers 854764 -Wcast-align 621102 -Wswitch-default 619302 -Wshadow 550802 -Wformat= 305056 -Wunused-macros 285910 -Wpointer-sign 184929 -Wsuggest-attribute=malloc 79124 -Wsuggest-attribute=pure 68675 -Wshift-overflow= 43685 -Wunused-const-variable= 40664 -Wimplicit-fallthrough= 37048 -Waggregate-return 35789 -Wunsuffixed-float-constants 35718 -Wsuggest-attribute=cold 28348 -Wsuggest-attribute=const 25315 -Wtype-limits 21354 -Woverride-init 19518 -Wmissing-prototypes 15989 -Wuninitialized 11801 -Wduplicated-branches 7969 -Wunused-but-set-variable 7826 -Woverlength-strings 6697 -Wstack-protector 6576 -Wformat-truncation= 5171 -Wfloat-conversion 4332 -Wduplicated-cond 2784 -Wformat-nonliteral 1972 -Wdouble-promotion 1500 -Wempty-body 1013 -Wformat-security 997 -Wsuggest-attribute=noreturn 984 -Wformat-overflow= 743 -Wvector-operation-performance 711 -Wcast-function-type 671 -Wstringop-truncation 647 -Wstack-usage= 623 -Wsuggest-attribute=format 393 -Wmaybe-uninitialized 319 -Wignored-qualifiers 307 -Wframe-larger-than= 304 -Wold-style-declaration 232 -Wlogical-op 190 -Wjump-misses-init 157 -Wframe-address 145 -Wfloat-equal 57 -Wundef 26 -Wstrict-overflow 16 -Wvla-larger-than= 14 -Wold-style-definition 6 -Wunused-but-set-parameter
I figured out the qnx4 warning in the end: https://godbolt.org/z/hvqjr3 struct qnx4_inode_entry { char di_status; union { struct { char di_fname[16]; char di_pad[32]; }; struct { char dl_fname[48]; }; }; }; int qnx4_readdir(struct qnx4_inode_entry *de) { if (!de->di_fname[0]) return 0; if (de->di_status) return __builtin_strnlen(de->di_fname, sizeof(de->di_fname)); else return __builtin_strnlen(de->dl_fname, sizeof(de->dl_fname)); return 0; } This produces <source>:22:16: warning: '__builtin_strnlen' specified bound 48 exceeds source size 16 [-Wstringop-overread] because the first access on the object seems to decide which layout is assumed. Changing (!de->di_fname[0]) to (!de->dl_fname[0]) shuts up the warning since that is a longer field. This is probably enough as a workaround, if you can confirm that the behavior of the compiler is also intentional for this input.
Note heuristically 0xe8ffc000 isn't likely such an offset from a NULL pointer object because the object would be quite large. The diagnostic could maybe also clarify that it assumes 0xe8ffc000 is an offsetted NULL pointer.
(In reply to Arnd Bergmann from comment #6) > I figured out the qnx4 warning in the end: https://godbolt.org/z/hvqjr3 The false positive is a known problem caused by redundancy elimination (the FRE/PRE passes) substituting one member for another when they both refer to the same address. It often happens with union members that share the same starting offset but can also happen with struct members in code that refers to both the end of one and the beginning of the next member with no intervening padding. There's no particular reason why it picks one or the other member (maybe it picks whichever it sees first in some order, I don't know), but the warning only triggers when it substitutes the smaller of the two members in a larger access. In the strnlen test case w/o sanitization it happens to pick the bigger member. This can be seen in the output of -fdump-tree-pre-details: Inserted pretmp_12 = &de_9(D)->D.2393.D.2392.dl_fname; in predecessor 3 (0004) Replaced &de_9(D)->D.2393.D.2392.dl_fname with pretmp_12 in all uses of _5 = &de_9(D)->D.2393.D.2392.dl_fname; Replaced &de_9(D)->D.2393.D.2389.di_fname with pretmp_12 in all uses of _3 = &de_9(D)->D.2393.D.2389.di_fname; Removing dead stmt _3 = &de_9(D)->D.2393.D.2389.di_fname; Removing dead stmt _5 = &de_9(D)->D.2393.D.2392.dl_fname; The instrumentation introduced by the sanitizers changes the IL in ways that affect the choices made by subsequent transformations. In this case, the sanitizer first inserts a call to __builtin___tsan_read1() to record the access to di_fname[0]. The sanitizers run after PRE but before some FRE iterations. The resulting IL from the thread sanitizer looks like this: <bb 2> [local count: 1073741824]: _13 = __builtin_return_address (0); __builtin___tsan_func_entry (_13); _5 = &de_9(D)->D.2390.D.2386.di_fname[0]; __builtin___tsan_read1 (_5); <<< record access to di_fname[0] _1 = de_9(D)->D.2390.D.2386.di_fname[0]; if (_1 == 0) goto <bb 7>; [34.00%] else goto <bb 3>; [66.00%] <bb 7> [local count: 365072224]: goto <bb 6>; [100.00%] <bb 3> [local count: 708669601]: _3 = &de_9(D)->di_status; __builtin___tsan_read1 (_3); _2 = de_9(D)->di_status; pretmp_12 = &de_9(D)->D.2390.D.2389.dl_fname; <<< temporary added by PRE if (_2 != 0) goto <bb 4>; [50.00%] else goto <bb 5>; [50.00%] <bb 4> [local count: 354334800]: _4 = __builtin_strnlen (pretmp_12, 16); _11 = (int) _4; goto <bb 6>; [100.00%] <bb 5> [local count: 354334800]: _6 = __builtin_strnlen (pretmp_12, 48); _10 = (int) _6; <bb 6> [local count: 1073741824]: # _7 = PHI <0(7), _11(4), _10(5)> __builtin___tsan_func_exit (); return _7; } The IL above is then changed by FRE which replaces dl_fname with di_fname: Value numbering stmt = pretmp_12 = &de_9(D)->D.2390.D.2389.dl_fname; Setting value number of pretmp_12 to _5 (changed) _5 is available for _5 Replaced &de_9(D)->D.2390.D.2389.dl_fname with _5 in all uses of pretmp_12 = &de_9(D)->D.2390.D.2389.dl_fname; The warning is then issued by the strlen pass that runs after FRE5 and that sees the IL below: <bb 2> [local count: 1073741824]: _13 = __builtin_return_address (0); __builtin___tsan_func_entry (_13); _5 = &de_9(D)->D.2393.D.2389.di_fname[0]; <<< inserted by sanitizer __builtin___tsan_read1 (_5); <<< record access to di_fname[0] _1 = de_9(D)->D.2393.D.2389.di_fname[0]; <<< inserted by FRE5 if (_1 == 0) goto <bb 6>; [34.00%] else goto <bb 3>; [66.00%] <bb 3> [local count: 708669601]: _3 = &de_9(D)->di_status; __builtin___tsan_read1 (_3); _2 = de_9(D)->di_status; if (_2 != 0) goto <bb 4>; [50.00%] else goto <bb 5>; [50.00%] <bb 4> [local count: 354334800]: _4 = strnlen (_5, 16); _11 = (int) _4; goto <bb 6>; [100.00%] <bb 5> [local count: 354334800]: _6 = strnlen (_5, 48); <<< -Wstringop-overread _10 = (int) _6; <bb 6> [local count: 1073741824]: # _7 = PHI <0(2), _11(4), _10(5)> __builtin___tsan_func_exit (); return _7; } I'm hoping to change PRE/FRE in GCC 12 to replace the member substitution with one involving &object + offsetof (type, memer). That will avoid the false positives in these cases with no adverse impact on optimization.
(In reply to Richard Biener from comment #7) > Note heuristically 0xe8ffc000 isn't likely such an offset from a NULL > pointer object because the object would be quite large. > > The diagnostic could maybe also clarify that it assumes 0xe8ffc000 is an > offsetted NULL pointer. I can do that in stage 1 when I convert the warning to use the access_ref class (that exposes this information). A better solution we discussed with Jeff is to issue -Wnonnull when a member access through a null pointer is first detected. Using something like __builtin_warning for that would help avoid false positives when this happens early on (in the test case in comment #1 that's in EVRP).
*** Bug 100190 has been marked as a duplicate of this bug. ***
*** Bug 100309 has been marked as a duplicate of this bug. ***
It looks to me separate bugs are mixed together here. For example I looked at the preallocate_pmd warning again and I don't think there is any union there. Also I noticed that when I replace the *foo[N] with **foo it disappears. So I think that is something different. So there seem to be instances where such warnings happen without union members. Perhaps that one (and perhaps some others) need to be reanalyzed. I also looked at the intel_pm.c and I think that one is a real kernel bug, where the field accessed is really too small. I'll submit a patch for that.
*** Bug 100680 has been marked as a duplicate of this bug. ***
I too have had what appears to be this bug, raised against a microkernel project. The logic is a test case involving interactions at the x86-64 lower canonical boundary, and reads like so: ... uint64_t *ptr = (void *)0x00007ffffffffff8ul; memcpy(ptr, "\x90\x90\xf\xbxen\x90", 8); ... This yields: include/xtf/libc.h:36:37: error: ‘__builtin_memcpy’ offset [0, 7] is out of the bounds [0, 0] [-Werror=array-bounds] 36 | #define memcpy(d, s, n) __builtin_memcpy(d, s, n) | ^~~~~~~~~~~~~~~~~~~~~~~~~ main.c:81:5: note: in expansion of macro ‘memcpy’ 81 | memcpy(ptr, "\x90\x90\xf\xbxen\x90", 8); | ^~~~~~ It is worth pointing out that it is common for kernels to have some virtual addresses derived from compile-time constants, notably the fixmap and frametable.
(In reply to Martin Sebor from comment #1) > For now, I recommend suppressing the warning either by #pragma GCC > diagnostic or by making the pointer volatile. Trying this with the code sample from comment 14 doesn't work. Not only does the -Werror=array-bounds diagnostic remain, a second is picked up from: passing argument 1 of ‘__builtin_memcpy’ discards ‘volatile’ qualifier from pointer target type [-Werror=discarded-qualifiers]
It's the pointer itself that needs to be volatile to keep GCC from determining its value. This shows the difference: $ cat pr99578-15.c && gcc -O2 -S -Wall pr99578-15.c void f (void) { void* ptr = (void *)0x00007ffffffffff8ul; __builtin_memcpy (ptr, "\x90\x90\xf\xbxen\x90", 8); // warning } void g (void) { void* volatile ptr = (void *)0x00007ffffffffff8ul; __builtin_memcpy (ptr, "\x90\x90\xf\xbxen\x90", 8); // okay } pr99578-15.c: In function ‘f’: pr99578-15.c:4:3: warning: ‘__builtin_memcpy’ offset [0, 7] is out of the bounds [0, 0] [-Warray-bounds] 4 | __builtin_memcpy (ptr, "\x90\x90\xf\xbxen\x90", 8); // warning | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Alternatively, store the address a global (modifiable) variable.
*** Bug 100834 has been marked as a duplicate of this bug. ***
*** Bug 102037 has been marked as a duplicate of this bug. ***
*** Bug 103768 has been marked as a duplicate of this bug. ***
(In reply to Andrew Pinski from comment #19) > *** Bug 103768 has been marked as a duplicate of this bug. *** It’d be great, if you could advise how to address the warning in SeaBIOS. In file included from src/fw/smm.c:18: src/fw/smm.c: In function 'smm_save_and_copy': src/string.h:23:16: warning: '__builtin_memcpy' offset [0, 511] is out of the bounds [0, 0] [-Warray-bounds] 23 | #define memcpy __builtin_memcpy src/fw/smm.c:148:5: note: in expansion of macro 'memcpy' 148 | memcpy(&smm->cpu, &initsmm->cpu, sizeof(smm->cpu)); | ^~~~~~
Created attachment 52193 [details] [SeaBIOS] [PATCH] smm: Suppress gcc array-bounds warnings For the record, I attach Kevin’s patch used to work around the issue in SeaBIOS. [1]: https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/thread/HLK3BHP2T3FN6FZ46BIPIK3VD5FOU74Z/
Re-confirmed on trunk. We need better heuristic on when to assume we're offsetting NULL or providing a static address.
*** Bug 104657 has been marked as a duplicate of this bug. ***
(In reply to Martin Sebor from comment #1) > The warning is by design. That just means the design is bad. Especially in the embedded world, using memory mapped IO at fixed addresses is very common and we really shouldn't force people to pessimize their code by adding volatile on the pointers etc. > Its purpose is to detect invalid accesses at > non-zero offsets to null pointers, like in the memset call below: > > struct S { int a, b[4]; }; > > void f (struct S *p) > { > if (p) return; > memset (p->b, 0, 4 * sizeof p->b); > } > > For now, I recommend suppressing the warning either by #pragma GCC > diagnostic or by making the pointer volatile. In the future, providing an > attribute to annotate constant addresses with (or extending the io() > attribute supported by some targets to all targets) might be another way to > avoid it. Then perhaps just add some flag on the INTEGER_CSTs created from folding &ptr->member into constant (seems currently that happens during {,e}vrp and dom) and only in the spots you warn if the INTEGER_CST has that flag set?
(In reply to Jakub Jelinek from comment #24) > (In reply to Martin Sebor from comment #1) > > The warning is by design. > > That just means the design is bad. Especially in the embedded world, using > memory mapped IO at fixed addresses is very common and we really shouldn't > force > people to pessimize their code by adding volatile on the pointers etc. No, the design is perfectly fine: it enforces the constraint in the standard that require pointers to point to live objects when they're used. Implementations can impose less restrictive constraints and implement behavior the standard leaves undefined by providing extensions. But to distinguish the two kinds of behavior they need to document the extensions. Failing to do that only leads to confusion and bugs (this code worked just fine in version X of GCC but version Y broke it!) Following this simple approach not only improves the quality of the implementation but also helps guide decisions about what's helpful to diagnose and what should be silently accepted. > > Its purpose is to detect invalid accesses at > > non-zero offsets to null pointers, like in the memset call below: > > > > struct S { int a, b[4]; }; > > > > void f (struct S *p) > > { > > if (p) return; > > memset (p->b, 0, 4 * sizeof p->b); > > } > > > > For now, I recommend suppressing the warning either by #pragma GCC > > diagnostic or by making the pointer volatile. In the future, providing an > > attribute to annotate constant addresses with (or extending the io() > > attribute supported by some targets to all targets) might be another way to > > avoid it. > > Then perhaps just add some flag on the INTEGER_CSTs created from folding > &ptr->member into constant (seems currently that happens during {,e}vrp and > dom) and only in the spots you warn if the INTEGER_CST has that flag set? In my opinion, code that deliberately uses invalid pointers (e.g., hardwired addresses) should be explicitly annotated, e.g., by some attribute. This approach has at least two advantages: 1) it makes the intent clear to the reader, and 2) by declaring the object it lets GCC enforce type safety as well as check for out-of-bounds access to it. GCC already provides two such attributes: the AVR address and io attributes. Rather than relying on heuristics I would suggest to make the address attribute (or one like it) available on all targets and use it for this purpose. (I started working on it last November but didn't finish it.)
(In reply to Martin Sebor from comment #25) > In my opinion, code that deliberately uses invalid pointers (e.g., hardwired > addresses) should be explicitly annotated, e.g., by some attribute. This > approach has at least two advantages: 1) it makes the intent clear to the > reader, and 2) by declaring the object it lets GCC enforce type safety as > well as check for out-of-bounds access to it. GCC already provides two such > attributes: the AVR address and io attributes. Rather than relying on > heuristics I would suggest to make the address attribute (or one like it) > available on all targets and use it for this purpose. (I started working on > it last November but didn't finish it.) That is nonsense. The amount of code in the wild that relies on (type *)CONSTANT working is insane, you can't annotate it all. And it has worked fine for decades. The pointers aren't invalid, they point to valid objects in the address space. POSIX supports MAP_FIXED for a reason (and in many embedded cases one doesn't even have an MMU and I/O or other special areas are mapped directly).
*** Bug 104655 has been marked as a duplicate of this bug. ***
*** Bug 104828 has been marked as a duplicate of this bug. ***
(In reply to Jakub Jelinek from comment #26) > That is nonsense. The amount of code in the wild that relies on (type > *)CONSTANT > working is insane, you can't annotate it all. And it has worked fine for > decades. The pointers aren't invalid, they point to valid objects in the > address space. > POSIX supports MAP_FIXED for a reason (and in many embedded cases one > doesn't even have an MMU and I/O or other special areas are mapped directly). A cast from integer to pointer is implementation defined behavior except for 1) 0 which must cast to NULL / nullptr 2) if the integer value was constructed by casting a pointer to integer of suitable size There is no garantee in the C standard that '(type *)CONSTANT' will actually point to the hardware address 'CONSTANT'. It's just how gcc happens to do it in most cases. So no, your code is not fine. It is fragile. It relies on implementation details of gcc. But lets not argue about that. Detecting NULL pointer access and offsets to it is a good thing, except where it isn't. It's unfortunate it also catches other stuff. Under AmigaOS the pointer to the exec.library (no program can work without that) is stored in address 4. So there isn't an universal value of "this is big enough not to be an offset to NULL". Detecting if an expression involves NULL might be hard. If it starts as NULL->member then it's easy. What about (&x - &x)+offsetof(X.member) or (uintptr_t)&x.member - (uintptr_t)&x or similar stuff you easily get with macros. On the other side (T*)0x45634534 should be easy to detect as not being NULL+offset. It's a literal. But the grey zone inbetween the easy cases might be to big to be useful. Alternatively an annotation for this would actually go nicely with another bug I reported: 'add feature to create a pointer to a fixed address as constexpr' [1]. The annotation would avoid the warning and also make it a pointer literal that can be used in constexpr (appart from accessing the address). It could also cause gcc to handle the case where CONSTANT can't just be cast to pointer and work. Like when using address authentication on ARMv8 CPUs, to name something modern. And the size of the object the pointer points to can be taken from its type, i.e. the pointer is to a single object and never an (infinite) array. If you want a pointer to an array then cast it to an array of the right size. -- [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104514
The Linux kernel is seeing more and more of these warnings and it's becoming a bit of a burden. For example: https://lore.kernel.org/linux-hardening/20220227195918.705219-1-keescook@chromium.org Given that this is a regression in behavior, and the kernel is not alone in tripping over this (9 duplicate bugs reported here already), can this be given greater priority? Would it be possible to add an option to just disable the "constant is a NULL pointer" logic directly?
I suppose we could move this warning under level 2 until this is handled better. -Warray-bounds already has two levels with level 2 being more noisy, and it might be useful to add a level to -Wstringop-overread as well. As I mentioned in comment #25 and elsewhere, I envisioned that code would annotate these hardwired addresses somehow, ideally using an attribute like addr or the Keil compiler's at (see below), or until one is added, using a workaround like your absolute_pointer(). I realize it means work, but I believe with the attribute the gain in type safety would make it worthwhile. Is that something the kernel developers could be trained to start using? (In full disclosure, I don't expect to have the cycles to work on the attribute anytime soon.) https://www.keil.com/support/man/docs/armcc/armcc_chr1359124981140.htm
I second Kees' request to please (permanently) add a flag to disable this inference and prevent GCC from making any assumptions about object sizes for pointers cast from integers. Ideally it should be a separate one-off flag rather than a level that needs to be set for a variety of individual warnings. This is affecting almost every systems programming project there is and going against 40 years of common C practice. The standard has never really specified a way to do MMIO hardware accesses, yet it's something we need to do in practice and casting integer literals to pointers is one of the main ways we do it. I don't think it's reasonable to suddenly start in 2022 to prosecute a "violation" that has been this prevalent for decades in programming practice. If you want to make these new safety checks available for those who want them that's fine, but please retain the option to stick with the existing behavior for those projects where this really causes a ton more problems than it could ever solve.
(In reply to Martin Sebor from comment #31) > As I mentioned in comment #25 and elsewhere, I envisioned that code would > annotate these hardwired addresses somehow, ideally using an attribute like > addr or the Keil compiler's at (see below), or until one is added, using a > workaround like your absolute_pointer(). I realize it means work, but I > believe with the attribute the gain in type safety would make it worthwhile. > Is that something the kernel developers could be trained to start using? > (In full disclosure, I don't expect to have the cycles to work on the > attribute anytime soon.) Whether or not it's reasonable to expect working code to be transitioned to a new feature, in the absence of such a feature (and no likelihood of it appearing any time soon) we should not be giving warnings for this code. The idea that it's zero-cost or zero-risk to go around sprinkling casts in working code that passes all its tests is foolish. Every cast added to silence a false positive warning has a risk of introducing a new problem and hiding a real bug in future.
(In reply to Goswin von Brederlow from comment #29) > There is no garantee in the C standard that '(type *)CONSTANT' will actually > point to the hardware address 'CONSTANT'. It's just how gcc happens to do it > in most cases. So no, your code is not fine. It is fragile. It relies on > implementation details of gcc. But lets not argue about that. Actually, lets. It relies on guaranteed behaviour of GCC: https://gcc.gnu.org/onlinedocs/gcc/Arrays-and-pointers-implementation.html That's not going to change, and neither is the fact that the Linux kernel depends on implementation-defined properties of GCC (where "implementation-defined" is used in the formal sense, not "just an implementation detail that might change tomorrow").
Created attachment 52643 [details] gcc12-pr99578-wip.patch What we could do is differentiate between the invalid constant addresses from pointer arithmetics on NULL or propagation of NULL into &ptr->field and valid (type *)constant used heavily in real-world code and accepted by all major compilers by representing the former internally as &MEM_REF[(void *)0B + offset] while the user (type *)constant as INTEGER_CSTs. That in full seems like a stage1 task because we'd need to ensure we don't regress much by doing that. Some things would be unavoidable (like avoiding SCCVN of those pointer INTEGER_CSTs vs. the &MEM_REF[(void *)0B + offset] form), e.g. equality comparison of &MEM_REF[(void *)0B + CST] vs. (void *)CST should be optimized etc., and we would need to force that form even on say (int *)0 + 24 etc. in the FEs. But we badly need to fix this for GCC 12 and backport to GCC 11. So this WIP patch treats addresses in the first page (a param defaulting to 4KB) temporarily as emitting the warnings as before in GCC 11/12, and only creates the &MEM_REF[(void *)0B + offset] form for larger offsets which will make them very rare. Some further tweaks will be needed on the gimple-array-bounds* etc. side so that we treat those &MEM_REF[(void *)0B + offset] as equivalent to the GCC 11 up to current trunk handling of (void *)offset, so that say on: struct S { int a, b[4]; }; struct T { int a, b[8192], c[4]; }; void foo (struct S *p) { if (p) return; __builtin_memset (p->b, 0, sizeof p->b); } void bar (struct T *p) { if (p) return; __builtin_memset (p->c, 0, sizeof p->c); } one gets the same warnings rather than different. On IRC Richi suggested just the params.opt and pointer-query.cc (compute_objsize_r) hunks which would only warn above in foo and not in bar.
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>: https://gcc.gnu.org/g:32ca611c42658948f1b8883994796f35e8b4e74d commit r12-7713-g32ca611c42658948f1b8883994796f35e8b4e74d Author: Jakub Jelinek <jakub@redhat.com> Date: Fri Mar 18 18:58:06 2022 +0100 Allow (void *) 0xdeadbeef accesses without warnings [PR99578] Starting with GCC11 we keep emitting false positive -Warray-bounds or -Wstringop-overflow etc. warnings on widely used *(type *)0x12345000 style accesses (or memory/string routines to such pointers). This is a standard programming style supported by all C/C++ compilers I've ever tried, used mostly in kernel or DSP programming, but sometimes also together with mmap MAP_FIXED when certain things, often I/O registers but could be anything else too are known to be present at fixed addresses. Such INTEGER_CST addresses can appear in code either because a user used it like that (in which case it is fine) or because somebody used pointer arithmetics (including &((struct whatever *)NULL)->field) on a NULL pointer. The middle-end warning code wrongly assumes that the latter case is what is very likely, while the former is unlikely and users should change their code. The following patch adds a min-pagesize param defaulting to 4KB, and treats INTEGER_CST addresses smaller than that as assumed results of pointer arithmetics from NULL while addresses equal or larger than that as expected user constant addresses. For GCC 13 we can represent results from pointer arithmetics on NULL using &MEM[(void*)0 + offset] instead of (void*)offset INTEGER_CSTs. 2022-03-18 Jakub Jelinek <jakub@redhat.com> PR middle-end/99578 PR middle-end/100680 PR tree-optimization/100834 * params.opt (--param=min-pagesize=): New parameter. * pointer-query.cc (compute_objsize_r) <case ARRAY_REF>: Formatting fix. (compute_objsize_r) <case INTEGER_CST>: Use maximum object size instead of zero for pointer constants equal or larger than min-pagesize. * gcc.dg/tree-ssa/pr99578-1.c: New test. * gcc.dg/pr99578-1.c: New test. * gcc.dg/pr99578-2.c: New test. * gcc.dg/pr99578-3.c: New test. * gcc.dg/pr100680.c: New test. * gcc.dg/pr100834.c: New test.
Fixed on the trunk so far, temporarily by differentiating between < 4KB addresses which are still handled like GCC 11 did for warning purposes, and >= 4KB addresses where we won't warn anymore. For GCC 13 there is a plan for more complete solution.
(In reply to Jonathan Wakely from comment #34) > (In reply to Goswin von Brederlow from comment #29) > > There is no garantee in the C standard that '(type *)CONSTANT' will actually > > point to the hardware address 'CONSTANT'. It's just how gcc happens to do it > > in most cases. So no, your code is not fine. It is fragile. It relies on > > implementation details of gcc. But lets not argue about that. > > Actually, lets. It relies on guaranteed behaviour of GCC: > https://gcc.gnu.org/onlinedocs/gcc/Arrays-and-pointers-implementation.html > That's not going to change, and neither is the fact that the Linux kernel > depends on implementation-defined properties of GCC (where > "implementation-defined" is used in the formal sense, not "just an > implementation detail that might change tomorrow"). Thank you for agreeing with me that "It relies on implementation details of gcc". That's exactly what I said.
The releases/gcc-11 branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>: https://gcc.gnu.org/g:91f7d7e1bb6827bf8e0b7ba7eb949953a5b1bd18 commit r11-9731-g91f7d7e1bb6827bf8e0b7ba7eb949953a5b1bd18 Author: Jakub Jelinek <jakub@redhat.com> Date: Fri Mar 18 18:58:06 2022 +0100 Allow (void *) 0xdeadbeef accesses without warnings [PR99578] Starting with GCC11 we keep emitting false positive -Warray-bounds or -Wstringop-overflow etc. warnings on widely used *(type *)0x12345000 style accesses (or memory/string routines to such pointers). This is a standard programming style supported by all C/C++ compilers I've ever tried, used mostly in kernel or DSP programming, but sometimes also together with mmap MAP_FIXED when certain things, often I/O registers but could be anything else too are known to be present at fixed addresses. Such INTEGER_CST addresses can appear in code either because a user used it like that (in which case it is fine) or because somebody used pointer arithmetics (including &((struct whatever *)NULL)->field) on a NULL pointer. The middle-end warning code wrongly assumes that the latter case is what is very likely, while the former is unlikely and users should change their code. The following patch adds a min-pagesize param defaulting to 4KB, and treats INTEGER_CST addresses smaller than that as assumed results of pointer arithmetics from NULL while addresses equal or larger than that as expected user constant addresses. For GCC 13 we can represent results from pointer arithmetics on NULL using &MEM[(void*)0 + offset] instead of (void*)offset INTEGER_CSTs. 2022-03-18 Jakub Jelinek <jakub@redhat.com> PR middle-end/99578 PR middle-end/100680 PR tree-optimization/100834 * params.opt (--param=min-pagesize=): New parameter. * builtins.c (compute_objsize_r) <case INTEGER_CST>: Use maximum object size instead of zero for pointer constants equal or larger than min-pagesize. * gcc.dg/tree-ssa/pr99578-1.c: New test. * gcc.dg/pr99578-1.c: New test. * gcc.dg/pr99578-2.c: New test. * gcc.dg/pr99578-3.c: New test. * gcc.dg/pr100680.c: New test. * gcc.dg/pr100834.c: New test. (cherry picked from commit 32ca611c42658948f1b8883994796f35e8b4e74d)
Can we close it as it was backported to gcc-11, Jakub?
Fixed for 11.3 too.
*** Bug 106699 has been marked as a duplicate of this bug. ***
(In reply to Jakub Jelinek from comment #37) > Fixed on the trunk so far, temporarily by differentiating between < 4KB > addresses which are still handled like GCC 11 did for warning purposes, and > >= 4KB addresses where we won't warn anymore. As noted by Andrew Pinski in bug #106699, comment #1, one has "to use --param=min-pagesize=0 if the first 4k is valid memory" to inhibit warnings for addresses below 4096.