Re: sparse: ARRAY_SIZE and sparse array initialization
On Fri, Mar 28, 2014 at 3:50 AM, Hans Verkuil hverk...@xs4all.nl wrote:
Is there any chance that the three issues I reported will be fixed? If not,
then I'll work around it in the kernel code.
Most likely it is a sparse issue. Can you generate a minimal stand alone
test case that expose this bug? I try to simplify it as following, but
it does not
reproduce the error.
#define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
static const char *v4l2_type_names[] = {
[1]= mmap,
[9] = userptr,
[2] = overlay,
[3] = dmabuf,
};
#define prt_names(a, arr) (((unsigned)(a)) ARRAY_SIZE(arr) ? arr[a]
: unknown)
extern void prt(const char *name);
static void v4l_print_requestbuffers(const void *arg, int write_only)
{
const struct v4l2_requestbuffers *p = arg;
prt(prt_names(3, v4l2_type_names));
}
If you have the test case, you are welcome to submit a patch to
add the test case.
Chris
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sparse: ARRAY_SIZE and sparse array initialization
Hi Chris,
On 03/30/2014 08:10 AM, Christopher Li wrote:
On Fri, Mar 28, 2014 at 3:50 AM, Hans Verkuil hverk...@xs4all.nl wrote:
Is there any chance that the three issues I reported will be fixed? If not,
then I'll work around it in the kernel code.
Most likely it is a sparse issue. Can you generate a minimal stand alone
test case that expose this bug? I try to simplify it as following, but
it does not
reproduce the error.
#define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
static const char *v4l2_type_names[] = {
[1]= mmap,
[9] = userptr,
[2] = overlay,
[3] = dmabuf,
};
#define prt_names(a, arr) (((unsigned)(a)) ARRAY_SIZE(arr) ? arr[a]
: unknown)
extern void prt(const char *name);
static void v4l_print_requestbuffers(const void *arg, int write_only)
{
const struct v4l2_requestbuffers *p = arg;
prt(prt_names(3, v4l2_type_names));
}
If you have the test case, you are welcome to submit a patch to
add the test case.
I experimented a bit more and it turned out that the use of EXPORT_SYMBOL
for the array is what causes the problem. Reproducible with this:
#include linux/kernel.h
#define prt_names(a, arr) (((unsigned)(a)) ARRAY_SIZE(arr) ? arr[a] :
unknown)
extern const char *v4l2_names[];
const char *v4l2_names[] = {
[1]= mmap,
[9] = userptr,
[2] = overlay,
[3] = dmabuf,
};
EXPORT_SYMBOL(v4l2_names);
extern void prt(const char *name);
static void v4l_print_requestbuffers(const void *arg, int write_only)
{
prt(prt_names(3, v4l2_names));
}
And with this sparse command:
sparse -nostdinc -isystem /usr/lib/gcc/x86_64-linux-gnu/4.8/include
-Iarch/x86/include -Iarch/x86/include/generated -Iinclude
-Iarch/x86/include/uapi -Iarch/x86/include/generated/uapi -Iinclude/uapi
-Iinclude/generated/uapi -include include/linux/kconfig.h -D__KERNEL__ x.c
If I remove EXPORT_SYMBOL all is well. EXPORT_SYMBOL expands to this:
extern typeof(v4l2_names) v4l2_names;
extern __attribute__((externally_visible)) void *__crc_v4l2_names
__attribute__((weak));
static const unsigned long __kcrctab_v4l2_names __attribute__((__used__))
__attribute__((section(___kcrctab + v4l2_names), unused)) = (unsigned
long) __crc_v4l2_names;
static const char __kstrtab_v4l2_names[]
__attribute__((section(__ksymtab_strings), aligned(1))) = v4l2_names;
extern const struct kernel_symbol __ksymtab_v4l2_names;
__attribute__((externally_visible)) const struct kernel_symbol
__ksymtab_v4l2_names __attribute__((__used__))
__attribute__((section(___ksymtab + v4l2_names), unused)) = {
(unsigned long)v4l2_names, __kstrtab_v4l2_names };
I did some more research and the key is the first line:
extern typeof(v4l2_names) v4l2_names;
If I add that to your test case:
static const char *v4l2_type_names[] = {
[1]= mmap,
[9] = userptr,
[2] = overlay,
[3] = dmabuf,
};
extern typeof(v4l2_type_names) v4l2_type_names;
Then I get the same error with your test case.
The smallest test case I can make is this:
== extern-array.c ==
extern const char *v4l2_type_names[];
const char *v4l2_type_names[] = {
test
};
extern const char *v4l2_type_names[];
static void test(void)
{
unsigned sz = sizeof(v4l2_type_names);
}
/*
* check-name: duplicate extern array
*
* check-error-start
* check-error-end
*/
== extern-array.c ==
If I leave out the both 'extern' declarations I get:
warning: symbol 'v4l2_type_names' was not declared. Should it be static?
Which is a correct warning.
If I leave out the second 'extern' only, then all is fine. If I add both
'extern' declarations, then I get:
error: cannot size expression
which is clearly a sparse bug somewhere.
Regards,
Hans
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sparse: ARRAY_SIZE and sparse array initialization
On Sun, Mar 30, 2014 at 5:03 AM, Hans Verkuil hverk...@xs4all.nl wrote: which is clearly a sparse bug somewhere. Yes. What is going on is that we create separate symbols for each declaration, and we tie them all together (and warn if they have conflicting types). But then when we look up a symbol, we only look at the latest one, so when we size the array, we look at that extern declaration, and don't see the size that was created with the initializer. I'll think about how to fix it cleanly. Expect a patch shortly. Linus -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sparse: ARRAY_SIZE and sparse array initialization
On 03/30/2014 06:48 PM, Linus Torvalds wrote: On Sun, Mar 30, 2014 at 5:03 AM, Hans Verkuil hverk...@xs4all.nl wrote: which is clearly a sparse bug somewhere. Yes. What is going on is that we create separate symbols for each declaration, and we tie them all together (and warn if they have conflicting types). But then when we look up a symbol, we only look at the latest one, so when we size the array, we look at that extern declaration, and don't see the size that was created with the initializer. I'll think about how to fix it cleanly. Expect a patch shortly. Great! Tomorrow I'll try to write test cases for the two other sparse problems I found. Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sparse: ARRAY_SIZE and sparse array initialization
On Sun, Mar 30, 2014 at 9:48 AM, Linus Torvalds torva...@linux-foundation.org wrote: But then when we look up a symbol, we only look at the latest one, so when we size the array, we look at that extern declaration, and don't see the size that was created with the initializer. Exactly. Sparse need to handle merging of incremental type declare. This is a long known sparse bug. I'll think about how to fix it cleanly. Expect a patch shortly. Wow, cool. I want to mention one special case of the type merging. It has some thing to do with scoping as well. When the scope ends, the incremental type added inside the scope will need to strip away as well. So it can not be blindly add to the original type without consider how to take that layer away later. This should be very exciting. Looking forward to the patch. Chris -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sparse: ARRAY_SIZE and sparse array initialization
On Sun, Mar 30, 2014 at 9:48 AM, Linus Torvalds torva...@linux-foundation.org wrote: I'll think about how to fix it cleanly. Expect a patch shortly. Ok, patch sent to linux-sparse mailing list. It fixes the particular cut-down test-case and seems pretty simple and straightforward, but is otherwise entirely untested, so who the hell knows.. Linus -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sparse: ARRAY_SIZE and sparse array initialization
Is there any chance that the three issues I reported will be fixed? If not,
then I'll work around it in the kernel code.
Regards,
Hans
On 03/15/2014 01:12 PM, Hans Verkuil wrote:
For the record: all these tests were done with a 3.14-rc5 kernel and sparse
compiled from the git tree as of today (version v0.5.0). The gcc version is
4.8.2.
Regards,
Hans
On 03/15/2014 01:09 PM, Hans Verkuil wrote:
Hmm, interesting. Twice 'sparse' in the same subject line with different
meanings :-)
This is another sparse error I get with drivers/media/v4l2-core/v4l2-ioctl.c:
drivers/media/v4l2-core/v4l2-ioctl.c:424:9: error: cannot size expression
(there are more of those in drivers/media, all with the same cause).
This sparse (the tool) error occurs because of sparse (C language) array
initialization
in combination with ARRAY_SIZE:
static const char *v4l2_memory_names[] = {
[V4L2_MEMORY_MMAP]= mmap,
[V4L2_MEMORY_USERPTR] = userptr,
[V4L2_MEMORY_OVERLAY] = overlay,
[V4L2_MEMORY_DMABUF] = dmabuf,
};
#define prt_names(a, arr) (((unsigned)(a)) ARRAY_SIZE(arr) ? arr[a] :
unknown)
static void v4l_print_requestbuffers(const void *arg, bool write_only)
{
const struct v4l2_requestbuffers *p = arg;
pr_cont(count=%d, type=%s, memory=%s\n,
p-count,
prt_names(p-type, v4l2_type_names),
prt_names(p-memory, v4l2_memory_names));
}
I could change v4l2_memory_names to:
static const char *v4l2_memory_names[V4L2_MEMORY_DMABUF + 1] = {
and the error goes away.
I'm actually not sure if this is a sparse bug or a feature.
If it is a feature then the error message is definitely wrong, since the
size is
perfectly well defined. As an aside: the error message is pretty vague IMHO.
Regards,
Hans
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-sparse in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sparse: ARRAY_SIZE and sparse array initialization
For the record: all these tests were done with a 3.14-rc5 kernel and sparse
compiled from the git tree as of today (version v0.5.0). The gcc version is
4.8.2.
Regards,
Hans
On 03/15/2014 01:09 PM, Hans Verkuil wrote:
Hmm, interesting. Twice 'sparse' in the same subject line with different
meanings :-)
This is another sparse error I get with drivers/media/v4l2-core/v4l2-ioctl.c:
drivers/media/v4l2-core/v4l2-ioctl.c:424:9: error: cannot size expression
(there are more of those in drivers/media, all with the same cause).
This sparse (the tool) error occurs because of sparse (C language) array
initialization
in combination with ARRAY_SIZE:
static const char *v4l2_memory_names[] = {
[V4L2_MEMORY_MMAP]= mmap,
[V4L2_MEMORY_USERPTR] = userptr,
[V4L2_MEMORY_OVERLAY] = overlay,
[V4L2_MEMORY_DMABUF] = dmabuf,
};
#define prt_names(a, arr) (((unsigned)(a)) ARRAY_SIZE(arr) ? arr[a] :
unknown)
static void v4l_print_requestbuffers(const void *arg, bool write_only)
{
const struct v4l2_requestbuffers *p = arg;
pr_cont(count=%d, type=%s, memory=%s\n,
p-count,
prt_names(p-type, v4l2_type_names),
prt_names(p-memory, v4l2_memory_names));
}
I could change v4l2_memory_names to:
static const char *v4l2_memory_names[V4L2_MEMORY_DMABUF + 1] = {
and the error goes away.
I'm actually not sure if this is a sparse bug or a feature.
If it is a feature then the error message is definitely wrong, since the size
is
perfectly well defined. As an aside: the error message is pretty vague IMHO.
Regards,
Hans
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
