sparse: ARRAY_SIZE and sparse array initialization

sparse: ARRAY_SIZE and sparse array initialization

[Hans Verkuil]

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

Re: sparse: ARRAY_SIZE and sparse array initialization

[Hans Verkuil]

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: sparse: ARRAY_SIZE and sparse array initialization

[Hans Verkuil]

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 majordomo@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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: sparse: ARRAY_SIZE and sparse array initialization

[Christopher Li]

On Fri, Mar 28, 2014 at 3:50 AM, Hans Verkuil <hverkuil@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

Re: sparse: ARRAY_SIZE and sparse array initialization

[Hans Verkuil]

Hi Chris,

On 03/30/2014 08:10 AM, Christopher Li wrote:
> On Fri, Mar 28, 2014 at 3:50 AM, Hans Verkuil <hverkuil@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

Re: sparse: ARRAY_SIZE and sparse array initialization

[Linus Torvalds]

On Sun, Mar 30, 2014 at 5:03 AM, Hans Verkuil <hverkuil@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

Re: sparse: ARRAY_SIZE and sparse array initialization

[Hans Verkuil]

On 03/30/2014 06:48 PM, Linus Torvalds wrote:
> On Sun, Mar 30, 2014 at 5:03 AM, Hans Verkuil <hverkuil@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

Re: sparse: ARRAY_SIZE and sparse array initialization

[Christopher Li]

On Sun, Mar 30, 2014 at 9:48 AM, Linus Torvalds
<torvalds@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

Re: sparse: ARRAY_SIZE and sparse array initialization

[Linus Torvalds]

On Sun, Mar 30, 2014 at 9:48 AM, Linus Torvalds
<torvalds@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