I see multiple new warnings for correct code in the Linux kernel for code that copies one array member into other members of the same array, reduced to: $ cat > test.c << EOF struct { int i; } a[8]; void f(void) { int i; for (i=1; i <8; i++) __builtin_memcpy(&a[i], &a[0], sizeof(a[0])); } EOF $ x86_64-linux-gcc-8.0.1 -c -Wall test.c test4.c: In function 'f': test4.c:8:3: warning: '__builtin_memcpy' accessing 4 bytes at offsets 0 and 0 overlaps 4 bytes at offset 0 [-Wrestrict] __builtin_memcpy(&a[i], &a[0], sizeof(a[0])); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This warning at least in its current shape doesn't belong into either -Wall or -W IMNSHO. if (TREE_CODE (expr) == ADDR_EXPR) { poly_int64 off; tree op = TREE_OPERAND (expr, 0); /* Determine the base object or pointer of the reference and its constant offset from the beginning of the base. */ base = get_addr_base_and_unit_offset (op, &off); HOST_WIDE_INT const_off; if (base && off.is_constant (&const_off)) { offrange[0] += const_off; offrange[1] += const_off; /* Stash the reference for offset validation. */ ref = op; /* Also stash the constant offset for offset validation. */ if (TREE_CODE (op) == COMPONENT_REF) refoff = const_off; } else { size = NULL_TREE; base = get_base_address (TREE_OPERAND (expr, 0)); } } is plain wrong. get_addr_base_and_unit_offset is solely for computation of a constant offset (or these days poly_int64 offset). Here you need to do get_inner_reference instead, and treat the poly_int64 bit offset (rather than byte offset) as the constant part and for the variable part try to use value ranges and handle casts/extensions in the expression too properly. I don't see how this warning can be enabled at all at -O0/-O1/-Og, unless you warn solely about cases where you can prove overalap, rather than warning just in case. And the general case of the warning should be, if I don't understand something (such as the base = get_base_address (TREE_OPERAND (expr, 0)); above, I punt on the warning, rather than just giving false positives. That is only a sure way to add the warning to the kill-list of projects that -Wno-* broken warnings, and for some users to stop using GCC.
(In reply to Arnd Bergmann from comment #0) Let me work on this. I tested the warning with the kernel but don't recall coming across this false positive. While I retry with the latest, how many of these do you see?
(In reply to Martin Sebor from comment #2) > (In reply to Arnd Bergmann from comment #0) > > Let me work on this. > > I tested the warning with the kernel but don't recall coming across this > false positive. While I retry with the latest, how many of these do you see? They are fairly rare, I have seen four or five now with randconfig builds over the last two days. It's possible that there is none in the default config.
Thanks. Just for reference in case more of these should pop up, I see two -Wrestrict instances in my build (below). The first one looks correct but the second one could be an instance of the false positive reported here. If you come across others please open new bugs (it would be helpful if I could reproduce your build so if you can share your config file and/or whatever else I might need that would be great). net/mac80211/cfg.c:2794:3: warning: ‘memcpy’ source argument is the same as destination [-Wrestrict] memcpy(pos, beacon->probe_resp, beacon->probe_resp_len); drivers/iommu/amd_iommu.c: In function ‘get_alias’: drivers/iommu/amd_iommu.c:310:4: warning: ‘memcpy’ accessing 32 bytes at offsets 0 and 0 overlaps 32 bytes at offset 0 [-Wrestrict] memcpy(amd_iommu_dev_table[pci_alias].data, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ amd_iommu_dev_table[devid].data, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sizeof(amd_iommu_dev_table[pci_alias].data)); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ PS The breakdown of all warnings I see is below (the Unique column gives the number of distinct origins of the warning, and the Files column the number of distinct originating files): Diagnostic Count Unique Files -Wattributes 2139 1 1 -Wattribute-alias 465 3 3 -Wpacked-not-aligned 45 17 3 -Wstringop-truncation 39 23 10 -Wformat-truncation= 21 21 15 -Wint-in-bool-context 10 10 2 -Wformat-overflow= 3 3 2 -Wstringop-overflow= 2 2 2 -Wrestrict 2 2 2 -Warray-bounds 2 2 2 -Wsizeof-pointer-memaccess 1 1 1 -Wnonnull 1 1 1
Here are some additional instances in the kernel. I'm currently trying to get a reliable build first and haven't got a log of all the messages, but there are a number of changes I did that are related, shutting up the -Wrestrict warning (some may be -Warray-bounds). Here are some false-positives: --- a/arch/x86/oprofile/nmi_int.c +++ b/arch/x86/oprofile/nmi_int.c @@ -282,9 +282,7 @@ static void mux_clone(int cpu) if (!has_mux()) return; - memcpy(per_cpu(cpu_msrs, cpu).multiplex, - per_cpu(cpu_msrs, 0).multiplex, - sizeof(struct op_msr) * model->num_virt_counters); + per_cpu(cpu_msrs, cpu).multiplex = per_cpu(cpu_msrs, 0).multiplex; } #else @@ -463,6 +461,7 @@ static int nmi_setup(void) if (!cpu) continue; +#ifdef CONFIG_SMP memcpy(per_cpu(cpu_msrs, cpu).counters, per_cpu(cpu_msrs, 0).counters, sizeof(struct op_msr) * model->num_counters); @@ -470,7 +469,7 @@ static int nmi_setup(void) memcpy(per_cpu(cpu_msrs, cpu).controls, per_cpu(cpu_msrs, 0).controls, sizeof(struct op_msr) * model->num_controls); - +#endif mux_clone(cpu); } --- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c @@ -1643,7 +1643,7 @@ static int cfg80211_rtw_scan(struct wiphy *wiphy spin_lock_bh(&pmlmepriv->lock); if (request->n_channels == 1) { for (i = 1;i<survey_times_for_one_ch;i++) - memcpy(&ch[i], &ch[0], sizeof(struct rtw_ieee80211_channel)); + ch[i] = ch[0]; _status = rtw_sitesurvey_cmd(padapter, ssid, RTW_SSID_SCAN_AMOUNT, ch, survey_times_for_one_ch); } else if (request->n_channels <= 4) { for (j =request->n_channels-1;j>= 0;j--) This one was weird, I suspect my change is incorrect: diff --git a/drivers/fmc/fmc-fakedev.c b/drivers/fmc/fmc-fakedev.c index 941d0930969a..0d322380d952 100644 --- a/drivers/fmc/fmc-fakedev.c +++ b/drivers/fmc/fmc-fakedev.c @@ -305,7 +305,7 @@ static int ff_init(void) /* Replicate the default eeprom for the max number of mezzanines */ for (i = 1; i < FF_MAX_MEZZANINES; i++) - memcpy(ff_eeimg[i], ff_eeimg[0], sizeof(ff_eeimg[0])); + memcpy(&ff_eeimg[i][0], &ff_eeimg[0][0], sizeof(ff_eeimg[0])); if (ff_nr_eeprom > ff_nr_dev) ff_nr_dev = ff_nr_eeprom; This one seems to be a kernel bug: --- a/kernel/debug/kdb/kdb_support.c +++ b/kernel/debug/kdb/kdb_support.c @@ -129,13 +129,13 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab) } if (i >= ARRAY_SIZE(kdb_name_table)) { debug_kfree(kdb_name_table[0]); - memcpy(kdb_name_table, kdb_name_table+1, + memmove(kdb_name_table, kdb_name_table+1, sizeof(kdb_name_table[0]) * (ARRAY_SIZE(kdb_name_table)-1)); } else { debug_kfree(knt1); knt1 = kdb_name_table[i]; - memcpy(kdb_name_table+i, kdb_name_table+i+1, + memmove(kdb_name_table+i, kdb_name_table+i+1, sizeof(kdb_name_table[0]) * (ARRAY_SIZE(kdb_name_table)-i-1)); }
I got one file that produces a rather cryptic warning related to this: In file included from /git/arm-soc/arch/x86/include/asm/page_32.h:35, from /git/arm-soc/arch/x86/include/asm/page.h:14, from /git/arm-soc/arch/x86/include/asm/thread_info.h:12, from /git/arm-soc/include/linux/thread_info.h:38, from /git/arm-soc/arch/x86/include/asm/preempt.h:7, from /git/arm-soc/include/linux/preempt.h:81, from /git/arm-soc/include/linux/spinlock.h:51, from /git/arm-soc/include/linux/seqlock.h:36, from /git/arm-soc/include/linux/time.h:6, from /git/arm-soc/include/linux/stat.h:22, from /git/arm-soc/include/linux/module.h:10, from /git/arm-soc/drivers/isdn/isdnloop/isdnloop.c:12: In function 'strcpy', inlined from 'isdnloop_parse_cmd' at /git/arm-soc/drivers/isdn/isdnloop/isdnloop.c:900:3: /git/arm-soc/include/linux/string.h:437:10: error: '__builtin_strcpy' accessing 0 or more bytes at offsets [36, 25] and 446 may overlap up to 0 bytes at offset [9223372036854775807, -9223372036854775808] [-Werror=restrict] return __builtin_strcpy(p, q); Not sure if gcc should try to avoid that warning or print something more helpful in that case. The isdnloop code itself is cryptic enough that I'm not surprised to see gcc get confused as well, and using strncpy() or strncpy() instead of strcpy() would avoid the warning and improve the source code. Possibly gcc should not warn about anything involving 'up to 0 bytes' though ;-)
(In reply to Arnd Bergmann from comment #6) > /git/arm-soc/include/linux/string.h:437:10: error: '__builtin_strcpy' > accessing 0 or more bytes at offsets [36, 25] and 446 may overlap up to 0 > bytes at offset [9223372036854775807, -9223372036854775808] This does suggest a bug in checker, similar to one that was fixed for memcpy not too long ago. The offset ranges should be in increasing order (i.e., [25, 36], not the other way around). Internally, they are represented as 128-bit numbers but when they're printed they are converted to 64-bit because GCC doesn't have support for printing anything bigger. The wrong order implies that the first offset crosses the PTRDIFF_MAX boundary (i.e., it's probably a union of two offsets: [36, PTRDIFF_MAX] and [PTRDIFF_MIN, 25]). It's possible that the strcpy checker has the same bug as the memcpy one. They are different because the memcpy checker warns only for definite overlaps and the one for strcpy and other string functions even for possible overlaps. I haven't yet been able to reproduce this in a small test case. I'll keep working on but it would help expedite the fix if you could attach a translation unit that reproduces it. Thanks for your help!
Created attachment 43295 [details] linux/drivers/isdn/isdnloop/isdnloop.c, preprocessed, compressed This is the preprocessed file that showed the funky -Wrestrict warning message: $ x86_64-linux-gcc-8.0.1 -Wall -O2 isdnloop.i -c -m32 -Wno-pointer-sign -Wno-unused In file included from /git/arm-soc/arch/x86/include/asm/page_32.h:35, from /git/arm-soc/arch/x86/include/asm/page.h:14, from /git/arm-soc/arch/x86/include/asm/thread_info.h:12, from /git/arm-soc/include/linux/thread_info.h:38, from /git/arm-soc/arch/x86/include/asm/preempt.h:7, from /git/arm-soc/include/linux/preempt.h:81, from /git/arm-soc/include/linux/spinlock.h:51, from /git/arm-soc/include/linux/seqlock.h:36, from /git/arm-soc/include/linux/time.h:6, from /git/arm-soc/include/linux/stat.h:22, from /git/arm-soc/include/linux/module.h:10, from /git/arm-soc/drivers/isdn/isdnloop/isdnloop.c:12: In function 'strcpy', inlined from 'isdnloop_parse_cmd' at /git/arm-soc/drivers/isdn/isdnloop/isdnloop.c:900:3, inlined from 'isdnloop_writecmd' at /git/arm-soc/drivers/isdn/isdnloop/isdnloop.c:989:5: /git/arm-soc/include/linux/string.h:437:10: warning: '__builtin_strcpy' accessing 0 or more bytes at offsets [36, 25] and 446 may overlap up to 0 bytes at offset [9223372036854775807, -9223372036854775808] [-Wrestrict] return __builtin_strcpy(p, q); ^~~~~~~~~~~~~~~~~~~~~~ I did not try to reduce the test case.
I found another false-positive -Wrestrict warning, did a manual reduction. Let me know if I should better open separate bugs for each test case, or you prefer to have them all here. $ aarch64-linux-gcc-8.0.1 -Wall -O2 -c sit.i sit.i: In function 'sit_init_net': sit.i:29:2: warning: 'memcpy' source argument is the same as destination [-Wrestrict] memcpy(&t->ip6rd, &t0->ip6rd, sizeof(t->ip6rd)); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ void *memcpy(void *, const void *, unsigned long ); struct ip_tunnel_6rd_parm { int relay_prefix; int prefixlen; int relay_prefixlen; }; struct netdevice { void *priv; }; struct ip_tunnel { struct netdevice *dev; struct ip_tunnel_6rd_parm ip6rd; }; struct sit_net { struct netdevice *fb_tunnel_dev; }; void ipip6_tunnel_clone_6rd(struct netdevice *dev, struct sit_net *sitn) { struct ip_tunnel *t = dev->priv; if (t->dev == sitn->fb_tunnel_dev) return; struct ip_tunnel *t0 = sitn->fb_tunnel_dev->priv; memcpy(&t->ip6rd, &t0->ip6rd, sizeof(t->ip6rd)); } int sit_init_net(struct sit_net *sitn, struct netdevice *fb_tunnel_dev) { sitn->fb_tunnel_dev = fb_tunnel_dev; ipip6_tunnel_clone_6rd(sitn->fb_tunnel_dev, sitn); return 0; }
Created attachment 43299 [details] Patch under test. I think that false positive is already being tracked in bug 83456. Let me add the test case to it. I'm testing the attached patch (it's an aggregate of the one posted here and the one posted for bug 83698 which is still waiting to be approved). Until I've verified the patch please add new test cases here and I'll triage them.
Patch for the bug in comment #0: https://gcc.gnu.org/ml/gcc-patches/2018-01/msg02396.html
(In reply to Arnd Bergmann from comment #8) I have manually reproduced a similar false positive in the small test case below it's not quite the same issue but I have a fix for both. They're both caused by similar oversights as the one in comment #0, except with slightly different root causes. In the translation unit from comment #8 the checker cannot reliably distinguish between offsets into different members of the same struct and offsets into the same member. In the case below the checker doesn't look deep enough through the array of arrays to see that p->a[i] need not be the same as p->a[0]. I expect to be done testing a fix for both tomorrow. If you have other test case/translation units please do add them here. This has been very helpful -- thanks again! $ cat z.c && gcc -O2 -S -Wall z.c struct S { char a[2][4]; }; void f (struct S *p, int i) { __builtin_strcpy (p->a[0], "012"); __builtin_strcpy (p->a[i] + 1, p->a[0]); } z.c: In function ‘f’: z.c:8:3: warning: ‘__builtin_strcpy’ accessing 4 bytes at offsets 1 and 0 overlaps 3 bytes at offset 1 [-Wrestrict] __builtin_strcpy (p->a[i] + 1, p->a[0]); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Updated patch with fixes for the false positives discussed in comment #12: https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00076.html
I applied the patches and seem to still get a warning for this: $ x86_64-linux-gcc-8.0.1 -Wall -O2 -c nmi_int.c nmi_int.c: In function 'nmi_setup': nmi_int.c:43:3: warning: 'memcpy' source argument is the same as destination [-Wrestrict] memcpy(per_cpu(cpu_msrs, cpu).counters, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ per_cpu(cpu_msrs, 0).counters, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sizeof(int) * model->num_counters); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ typedef unsigned long __kernel_size_t; extern void * memcpy(void *,const void *,__kernel_size_t); struct op_msrs { int *counters; }; #define for_each_possible_cpu(cpu) for_each_cpu((cpu), cpu_possible_mask) #define for_each_cpu(cpu, mask) for ((cpu) = 0; (cpu) < 1; (cpu)++,(void)mask) extern struct cpumask __cpu_possible_mask; #define cpu_possible_mask ((const struct cpumask *)&__cpu_possible_mask) #define DEFINE_PER_CPU(type, name) __typeof__(type) name #define per_cpu_ptr(ptr, cpu) ({ (void)(cpu); ptr; }) #define raw_cpu_ptr(ptr) per_cpu_ptr(ptr, 0) #define per_cpu(var, cpu) (*per_cpu_ptr(&(var), cpu)) extern void *pcpu_base_addr; extern const unsigned long *pcpu_unit_offsets; struct op_x86_model_spec { unsigned int num_counters; }; static struct op_x86_model_spec *model; static DEFINE_PER_CPU(struct op_msrs, cpu_msrs); int nmi_setup(void) { int err = 0; int cpu; for_each_possible_cpu(cpu) { if (!cpu) continue; memcpy(per_cpu(cpu_msrs, cpu).counters, per_cpu(cpu_msrs, 0).counters, sizeof(int) * model->num_counters); } return err; } In this code, we do copy from a variable onto itself, but only in a dead branch, here because the for_each_possible_cpu() and per_cpu() macros degrade to trivial wrappers on an non-SMP build.
(In reply to Arnd Bergmann from comment #14) > I applied the patches and seem to still get a warning for this I also just got the one from comment #9 again and found that the reduced test case is still affected (and not claimed to be fixed by any of the patches, so that's my fault for not checking).
Thanks for the test case in comment #14. I've reproduced the warning and will look into it. The false positive from comment #9 isn't fixed yet. I'm assuming it's due to the same root cause as bug 83456 that I'm tracking separately. There's no way to avoid that one other than by disabling the warning in a whole set of cases. I'm trying to decide if trading the false positives for the unavoidable false negatives make sense.
I've reduced the test case from comment #14 to the one below. I'm inclined to think the warning is justified. As you say, the code clearly does use the same pointer for both the source and the destination, so the code is strictly not valid. GCC ultimately eliminates the memcpy call and the whole loop, but not before it notices the argument is the same. The warning is issued in the same places as the one for bug 83456 so if a decision is made to fix that bug this warning will also disappear. $ cat t.c && gcc -O2 -S -Wall t.c extern void* memcpy (void*, const void*, __SIZE_TYPE__); char a[4]; void f (unsigned n) { for (int i = 0; i < 1; i++) { if (!i) continue; memcpy (a, a, n); } } t.c: In function ‘f’: t.c:12:7: warning: ‘memcpy’ source argument is the same as destination [-Wrestrict] memcpy (a, a, n); ^~~~~~~~~~~~~~~~
Author: msebor Date: Tue Feb 20 20:22:01 2018 New Revision: 257860 URL: https://gcc.gnu.org/viewcvs?rev=257860&root=gcc&view=rev Log: PR middle-end/84095 - false-positive -Wrestrict warnings for memcpy within array gcc/ChangeLog: PR middle-end/84095 * gimple-ssa-warn-restrict.c (builtin_memref::extend_offset_range): New. (builtin_memref::set_base_and_offset): Same. Handle inner references. (builtin_memref::builtin_memref): Factor out parts into set_base_and_offset and call it. gcc/testsuite/ChangeLog: PR middle-end/84095 * c-c++-common/Warray-bounds-3.c: Adjust text of expected warnings. * c-c++-common/Wrestrict.c: Same. * gcc.dg/Wrestrict-6.c: Same. * gcc.dg/Warray-bounds-27.c: New test. * gcc.dg/Wrestrict-8.c: New test. * gcc.dg/Wrestrict-9.c: New test. * gcc.dg/pr84095.c: New test. Added: trunk/gcc/testsuite/gcc.dg/Warray-bounds-27.c trunk/gcc/testsuite/gcc.dg/Wrestrict-8.c trunk/gcc/testsuite/gcc.dg/Wrestrict-9.c trunk/gcc/testsuite/gcc.dg/pr84095.c Modified: trunk/gcc/ChangeLog trunk/gcc/gimple-ssa-warn-restrict.c trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/c-c++-common/Warray-bounds-3.c trunk/gcc/testsuite/c-c++-common/Wrestrict.c trunk/gcc/testsuite/gcc.dg/Wrestrict-6.c
Fixed in r257860. (The remaining false positives are being tracked in bug 83456.)