[PATCH 1/2] dma/pool: trivial: add semicolon after label attributes

[PATCH 1/2] dma/pool: trivial: add semicolon after label attributes

[Chunhui He]

The gcc document says label attributes are ambiguous if they are
not immediately followed by a semicolon. Although the ambiguity
does not arise in C90/99, it would be better to add it.

Link: https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html#Label-Attributes-2
Signed-off-by: Chunhui He <hchunhui@mail.ustc.edu.cn>
---
 kernel/dma/pool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 1acec2e22827..f99f02b88c40 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -136,7 +136,7 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
 #ifdef CONFIG_DMA_DIRECT_REMAP
 	dma_common_free_remap(addr, pool_size);
 #endif
-free_page: __maybe_unused
+free_page: __maybe_unused;
 	__free_pages(page, order);
 out:
 	return ret;
-- 
2.39.2

Re: [PATCH 1/2] dma/pool: trivial: add semicolon after label attributes

[Robin Murphy]

On 26/08/2023 9:53 am, Chunhui He wrote:
> The gcc document says label attributes are ambiguous if they are
> not immediately followed by a semicolon. Although the ambiguity
> does not arise in C90/99, it would be better to add it.

AFAICS, what that clearly says is that *C++* label attributes can be 
ambiguous. This is not C++ code. Even in C11, declarations still cannot 
be labelled, so it should still be the case that, per the same GCC 
documentation, "the ambiguity does not arise". And even if the language 
did allow it, an inline declaration at that point at the end of a 
function would be downright weird and against the kernel coding style 
anyway.

So, I don't really see what's "better" about cluttering up C code with 
unnecessary C++isms; it's just weird noise to me. The only thing I think 
it *does* achieve is introduce the chance that the static checker 
brigade eventually identifies a redundant semicolon and we get more 
patches to remove it again.

Thanks,
Robin.

> Link: https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html#Label-Attributes-2
> Signed-off-by: Chunhui He <hchunhui@mail.ustc.edu.cn>
> ---
>   kernel/dma/pool.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> index 1acec2e22827..f99f02b88c40 100644
> --- a/kernel/dma/pool.c
> +++ b/kernel/dma/pool.c
> @@ -136,7 +136,7 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
>   #ifdef CONFIG_DMA_DIRECT_REMAP
>   	dma_common_free_remap(addr, pool_size);
>   #endif
> -free_page: __maybe_unused
> +free_page: __maybe_unused;
>   	__free_pages(page, order);
>   out:
>   	return ret;

Re: [PATCH 1/2] dma/pool: trivial: add semicolon after label attributes

[Christoph Hellwig]

On Tue, Aug 29, 2023 at 03:22:22PM +0100, Robin Murphy wrote:
> AFAICS, what that clearly says is that *C++* label attributes can be 
> ambiguous. This is not C++ code. Even in C11, declarations still cannot be 
> labelled, so it should still be the case that, per the same GCC 
> documentation, "the ambiguity does not arise". And even if the language did 
> allow it, an inline declaration at that point at the end of a function 
> would be downright weird and against the kernel coding style anyway.
>
> So, I don't really see what's "better" about cluttering up C code with 
> unnecessary C++isms; it's just weird noise to me. The only thing I think it 
> *does* achieve is introduce the chance that the static checker brigade 
> eventually identifies a redundant semicolon and we get more patches to 
> remove it again.

Agreed.  Even more importantly that attribute looks rather questionable
to start with as it can be dropped by just moving the #endif a little:

diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 1acec2e228273f..da03c4a57cebe3 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -135,8 +135,8 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
 remove_mapping:
 #ifdef CONFIG_DMA_DIRECT_REMAP
 	dma_common_free_remap(addr, pool_size);
+free_page:
 #endif
-free_page: __maybe_unused
 	__free_pages(page, order);
 out:
 	return ret;

Re: [PATCH 1/2] dma/pool: trivial: add semicolon after label attributes

[Robin Murphy]

On 29/08/2023 4:12 pm, Christoph Hellwig wrote:
> On Tue, Aug 29, 2023 at 03:22:22PM +0100, Robin Murphy wrote:
>> AFAICS, what that clearly says is that *C++* label attributes can be
>> ambiguous. This is not C++ code. Even in C11, declarations still cannot be
>> labelled, so it should still be the case that, per the same GCC
>> documentation, "the ambiguity does not arise". And even if the language did
>> allow it, an inline declaration at that point at the end of a function
>> would be downright weird and against the kernel coding style anyway.
>>
>> So, I don't really see what's "better" about cluttering up C code with
>> unnecessary C++isms; it's just weird noise to me. The only thing I think it
>> *does* achieve is introduce the chance that the static checker brigade
>> eventually identifies a redundant semicolon and we get more patches to
>> remove it again.
> 
> Agreed.  Even more importantly that attribute looks rather questionable
> to start with as it can be dropped by just moving the #endif a little:
> 
> diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> index 1acec2e228273f..da03c4a57cebe3 100644
> --- a/kernel/dma/pool.c
> +++ b/kernel/dma/pool.c
> @@ -135,8 +135,8 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
>   remove_mapping:
>   #ifdef CONFIG_DMA_DIRECT_REMAP
>   	dma_common_free_remap(addr, pool_size);
> +free_page:
>   #endif
> -free_page: __maybe_unused
>   	__free_pages(page, order);
>   out:
>   	return ret;

Oh, indeed - I hadn't really looked at the context itself. My 
non-exhaustive grep skills show a couple of hundred instances of 
label-above-#endif vs. three (!) instances of __maybe_unused, so ack to 
making that cleanup to just remove the question entirely.

Cheers,
Robin.

Re: [PATCH 1/2] dma/pool: trivial: add semicolon after label attributes

[Chunhui He]

On Tue, 29 Aug 2023 16:28:05 +0100, Robin Murphy <robin.murphy@arm.com> wrote:
> On 29/08/2023 4:12 pm, Christoph Hellwig wrote:
>> On Tue, Aug 29, 2023 at 03:22:22PM +0100, Robin Murphy wrote:
>>> AFAICS, what that clearly says is that *C++* label attributes can be
>>> ambiguous. This is not C++ code. Even in C11, declarations still
>>> cannot be
>>> labelled, so it should still be the case that, per the same GCC
>>> documentation, "the ambiguity does not arise". And even if the
>>> language did
>>> allow it, an inline declaration at that point at the end of a function
>>> would be downright weird and against the kernel coding style anyway.
>>>
>>> So, I don't really see what's "better" about cluttering up C code with
>>> unnecessary C++isms; it's just weird noise to me. The only thing I
>>> think it
>>> *does* achieve is introduce the chance that the static checker brigade
>>> eventually identifies a redundant semicolon and we get more patches to
>>> remove it again.

Inline declaration is a GNU C extension, so the ambiguity may arise.
Adding ';' makes the compiler easier to parse correctly, so I say
"better". The commit 13a453c241b78934a945b1af572d0533612c9bd1
(sched/fair: Add ';' after label attributes) also says the same.

>> Agreed.  Even more importantly that attribute looks rather
>> questionable
>> to start with as it can be dropped by just moving the #endif a little:
>> diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
>> index 1acec2e228273f..da03c4a57cebe3 100644
>> --- a/kernel/dma/pool.c
>> +++ b/kernel/dma/pool.c
>> @@ -135,8 +135,8 @@ static int atomic_pool_expand(struct gen_pool
>> *pool, size_t pool_size,
>>   remove_mapping:
>>   #ifdef CONFIG_DMA_DIRECT_REMAP
>>   	dma_common_free_remap(addr, pool_size);
>> +free_page:
>>   #endif
>> -free_page: __maybe_unused
>>   	__free_pages(page, order);
>>   out:
>>   	return ret;
> 
> Oh, indeed - I hadn't really looked at the context itself. My
> non-exhaustive grep skills show a couple of hundred instances of
> label-above-#endif vs. three (!) instances of __maybe_unused, so ack
> to making that cleanup to just remove the question entirely.
> 
> Cheers,
> Robin.

I agree label-above-#endif remove the question entirely.

Cheers,
Chunhui.

Re: [PATCH 1/2] dma/pool: trivial: add semicolon after label attributes

[Robin Murphy]

On 2023-08-31 12:59, Chunhui He wrote:
> 
> On Tue, 29 Aug 2023 16:28:05 +0100, Robin Murphy <robin.murphy@arm.com> wrote:
>> On 29/08/2023 4:12 pm, Christoph Hellwig wrote:
>>> On Tue, Aug 29, 2023 at 03:22:22PM +0100, Robin Murphy wrote:
>>>> AFAICS, what that clearly says is that *C++* label attributes can be
>>>> ambiguous. This is not C++ code. Even in C11, declarations still
>>>> cannot be
>>>> labelled, so it should still be the case that, per the same GCC
>>>> documentation, "the ambiguity does not arise". And even if the
>>>> language did
>>>> allow it, an inline declaration at that point at the end of a function
>>>> would be downright weird and against the kernel coding style anyway.
>>>>
>>>> So, I don't really see what's "better" about cluttering up C code with
>>>> unnecessary C++isms; it's just weird noise to me. The only thing I
>>>> think it
>>>> *does* achieve is introduce the chance that the static checker brigade
>>>> eventually identifies a redundant semicolon and we get more patches to
>>>> remove it again.
> 
> Inline declaration is a GNU C extension, so the ambiguity may arise.
> Adding ';' makes the compiler easier to parse correctly, so I say
> "better". The commit 13a453c241b78934a945b1af572d0533612c9bd1
> (sched/fair: Add ';' after label attributes) also says the same.

And that commit was also wrong. Nobody suggested C11 doesn't support 
inline declarations - it demonstrably does - the fact in question is 
that *attributes* on declarations is a C++ thing and not valid in C:

~/src/linux$ git diff
diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 1acec2e22827..e1354235cb9c 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -137,7 +137,8 @@ static int atomic_pool_expand(struct gen_pool *pool, 
size_t pool_size,
         dma_common_free_remap(addr, pool_size);
  #endif
  free_page: __maybe_unused
-       __free_pages(page, order);
+       int x = order;
+       __free_pages(page, x);
  out:
         return ret;
  }
~/src/linux$ make -j32
   CALL    scripts/checksyscalls.sh
   CC      kernel/dma/pool.o
kernel/dma/pool.c: In function ‘atomic_pool_expand’:
kernel/dma/pool.c:140:2: error: a label can only be part of a statement 
and a declaration is not a statement
   140 |  int x = order;
       |  ^~~
make[4]: *** [scripts/Makefile.build:243: kernel/dma/pool.o] Error 1
make[3]: *** [scripts/Makefile.build:480: kernel/dma] Error 2
make[2]: *** [scripts/Makefile.build:480: kernel] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/home/robmur01/src/linux/Makefile:2032: .] Error 2
make: *** [Makefile:234: __sub-make] Error 2


Thanks,
Robin.

Re: [PATCH 1/2] dma/pool: trivial: add semicolon after label attributes

[Robin Murphy]

On 2023-09-01 09:56, Robin Murphy wrote:
> On 2023-08-31 12:59, Chunhui He wrote:
>>
>> On Tue, 29 Aug 2023 16:28:05 +0100, Robin Murphy 
>> <robin.murphy@arm.com> wrote:
>>> On 29/08/2023 4:12 pm, Christoph Hellwig wrote:
>>>> On Tue, Aug 29, 2023 at 03:22:22PM +0100, Robin Murphy wrote:
>>>>> AFAICS, what that clearly says is that *C++* label attributes can be
>>>>> ambiguous. This is not C++ code. Even in C11, declarations still
>>>>> cannot be
>>>>> labelled, so it should still be the case that, per the same GCC
>>>>> documentation, "the ambiguity does not arise". And even if the
>>>>> language did
>>>>> allow it, an inline declaration at that point at the end of a function
>>>>> would be downright weird and against the kernel coding style anyway.
>>>>>
>>>>> So, I don't really see what's "better" about cluttering up C code with
>>>>> unnecessary C++isms; it's just weird noise to me. The only thing I
>>>>> think it
>>>>> *does* achieve is introduce the chance that the static checker brigade
>>>>> eventually identifies a redundant semicolon and we get more patches to
>>>>> remove it again.
>>
>> Inline declaration is a GNU C extension, so the ambiguity may arise.
>> Adding ';' makes the compiler easier to parse correctly, so I say
>> "better". The commit 13a453c241b78934a945b1af572d0533612c9bd1
>> (sched/fair: Add ';' after label attributes) also says the same.
> 
> And that commit was also wrong. Nobody suggested C11 doesn't support 
> inline declarations - it demonstrably does - the fact in question is 
> that *attributes* on declarations is a C++ thing and not valid in C:

Argh, sorry, s/attributes/labels/

/me goes to make more coffee...

Robin.

> ~/src/linux$ git diff
> diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> index 1acec2e22827..e1354235cb9c 100644
> --- a/kernel/dma/pool.c
> +++ b/kernel/dma/pool.c
> @@ -137,7 +137,8 @@ static int atomic_pool_expand(struct gen_pool *pool, 
> size_t pool_size,
>          dma_common_free_remap(addr, pool_size);
>   #endif
>   free_page: __maybe_unused
> -       __free_pages(page, order);
> +       int x = order;
> +       __free_pages(page, x);
>   out:
>          return ret;
>   }
> ~/src/linux$ make -j32
>    CALL    scripts/checksyscalls.sh
>    CC      kernel/dma/pool.o
> kernel/dma/pool.c: In function ‘atomic_pool_expand’:
> kernel/dma/pool.c:140:2: error: a label can only be part of a statement 
> and a declaration is not a statement
>    140 |  int x = order;
>        |  ^~~
> make[4]: *** [scripts/Makefile.build:243: kernel/dma/pool.o] Error 1
> make[3]: *** [scripts/Makefile.build:480: kernel/dma] Error 2
> make[2]: *** [scripts/Makefile.build:480: kernel] Error 2
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [/home/robmur01/src/linux/Makefile:2032: .] Error 2
> make: *** [Makefile:234: __sub-make] Error 2
> 
> 
> Thanks,
> Robin.
>