[Question] test_skeleton selftest build failure on LLVM main

[Question] test_skeleton selftest build failure on LLVM main

[Eduard Zingerman]

Hi Yonghong, Jose,

I've noticed today that LLVM main started producing an error when
compiling selftest test_skeleton.c:

    progs/test_skeleton.c:46:20: error: 'in_dynarr_sz' causes a section type conflict with 'in_dynarr'
       46 | const volatile int in_dynarr_sz SEC(".rodata.dyn");
          |                    ^
    progs/test_skeleton.c:47:20: note: declared here
       47 | const volatile int in_dynarr[4] SEC(".rodata.dyn") = { -1, -2, -3, -4 };
          |                    ^
    1 error generated.
      CLNG-BPF [test_maps] test_sk_storage_trace_itself.bpf.o
    make: *** [Makefile:594: /home/eddy/work/bpf-next/tools/testing/selftests/bpf/test_skeleton.bpf.o] Error 1

The code in question looks as follows:

    ...
    const volatile int in_dynarr_sz SEC(".rodata.dyn");
    const volatile int in_dynarr[4] SEC(".rodata.dyn") = { -1, -2, -3, -4 };
    ...

In fact, it could be simplified to the following example:

    #define SEC(n) __attribute__((section(n)))

    const int with_init SEC("foo") = 1;
    const int no_init SEC("foo");

And error is reported for x86 build as well:

    $ clang -c t.c -o /dev/null
    t.c:4:11: error: 'no_init' causes a section type conflict with 'with_init'
        4 | const int no_init SEC("foo");
          |           ^
    t.c:3:11: note: declared here
        3 | const int with_init SEC("foo") = 1;
          |           ^
    1 error generated.

The error occurs because clang infers "read only" attribute for
section "foo" when `with_init` is processed and "read/write"
attributes for section "foo" when `no_init` is processed.
The attributes do not match and error is reported.
(See Sema::UnifySection, `diag::err_section_conflict` diagnostic).

The culprit is revision [1] which landed today. The main focus of that
revision is C++ and handling of structure fields marked as `mutable`.
However, it also adds a new requirement: for global value to be
considered "read only" it must have an initializer
(the `var->hasInit()` check in [2]).

GCC can handle the example above w/o any issues.
The relevant part of the C standard [3] is "6.7.3 Type qualifiers",
but it does not discuss sections, the only section-related sentence
that I found is:

> 160) The implementation can place a const object that is not
>      volatile in a read-only region of storage. Moreover, the
>      implementation need not allocate storage for such an object if
>      its address is never used.

Which does not make example at hand invalid.

Although `const` values w/o initializer do seem strange they might
have some sense if, say, linker materializes these definitions with
something useful.

Thus, it appears to me that:
- test_skeleton.c is ok and should not be changed;
- revision [1] introduced a bug and I should bring it up with upstream.
  
What do you think?

Thanks,
Eduard

[1] https://reviews.llvm.org/D156726
[2] https://github.com/llvm/llvm-project/compare/main...llvm-premerge-tests:llvm-project:phab-diff-550097#diff-edac6256ac508912a16d0165b2f8cf37123dc2f40a147dca49a34c33f1db13ddR14366
[3] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3088.pdf

Re: [Question] test_skeleton selftest build failure on LLVM main

[Yonghong Song]

On 8/15/23 4:20 PM, Eduard Zingerman wrote:
> Hi Yonghong, Jose,
> 
> I've noticed today that LLVM main started producing an error when
> compiling selftest test_skeleton.c:
> 
>      progs/test_skeleton.c:46:20: error: 'in_dynarr_sz' causes a section type conflict with 'in_dynarr'
>         46 | const volatile int in_dynarr_sz SEC(".rodata.dyn");
>            |                    ^
>      progs/test_skeleton.c:47:20: note: declared here
>         47 | const volatile int in_dynarr[4] SEC(".rodata.dyn") = { -1, -2, -3, -4 };
>            |                    ^
>      1 error generated.
>        CLNG-BPF [test_maps] test_sk_storage_trace_itself.bpf.o
>      make: *** [Makefile:594: /home/eddy/work/bpf-next/tools/testing/selftests/bpf/test_skeleton.bpf.o] Error 1
> 
> The code in question looks as follows:
> 
>      ...
>      const volatile int in_dynarr_sz SEC(".rodata.dyn");
>      const volatile int in_dynarr[4] SEC(".rodata.dyn") = { -1, -2, -3, -4 };
>      ...
> 
> In fact, it could be simplified to the following example:
> 
>      #define SEC(n) __attribute__((section(n)))
> 
>      const int with_init SEC("foo") = 1;
>      const int no_init SEC("foo");
> 
> And error is reported for x86 build as well:
> 
>      $ clang -c t.c -o /dev/null
>      t.c:4:11: error: 'no_init' causes a section type conflict with 'with_init'
>          4 | const int no_init SEC("foo");
>            |           ^
>      t.c:3:11: note: declared here
>          3 | const int with_init SEC("foo") = 1;
>            |           ^
>      1 error generated.
> 
> The error occurs because clang infers "read only" attribute for
> section "foo" when `with_init` is processed and "read/write"
> attributes for section "foo" when `no_init` is processed.
> The attributes do not match and error is reported.
> (See Sema::UnifySection, `diag::err_section_conflict` diagnostic).
> 
> The culprit is revision [1] which landed today. The main focus of that
> revision is C++ and handling of structure fields marked as `mutable`.
> However, it also adds a new requirement: for global value to be
> considered "read only" it must have an initializer
> (the `var->hasInit()` check in [2]).
> 
> GCC can handle the example above w/o any issues.
> The relevant part of the C standard [3] is "6.7.3 Type qualifiers",
> but it does not discuss sections, the only section-related sentence
> that I found is:
> 
>> 160) The implementation can place a const object that is not
>>       volatile in a read-only region of storage. Moreover, the
>>       implementation need not allocate storage for such an object if
>>       its address is never used.
> 
> Which does not make example at hand invalid.
> 
> Although `const` values w/o initializer do seem strange they might
> have some sense if, say, linker materializes these definitions with
> something useful.
> 
> Thus, it appears to me that:
> - test_skeleton.c is ok and should not be changed;
> - revision [1] introduced a bug and I should bring it up with upstream.

Thanks Eduard.
Please go ahead bringing the issue to upstream as a comment on [1].
Although the issue can be trivial fixed by add '= 0' initialization,
it will be still good to clarify with upstream.

I remember that we discussed 'const volatile' thing with gcc as well
and agreed it should be put into .rodata section. But I lost that
email communication. I am not sure how initialization will change this.

>    
> What do you think?
> 
> Thanks,
> Eduard
> 
> [1] https://reviews.llvm.org/D156726
> [2] https://github.com/llvm/llvm-project/compare/main...llvm-premerge-tests:llvm-project:phab-diff-550097#diff-edac6256ac508912a16d0165b2f8cf37123dc2f40a147dca49a34c33f1db13ddR14366
> [3] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3088.pdf
> 
>