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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f3bec3b9-e938-bea4-f89f-b0b698c4e302@arm.com>
Date:   Fri, 1 Sep 2023 09:56:32 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Chunhui He <hchunhui@...l.ustc.edu.cn>
Cc:     hch@....de, m.szyprowski@...sung.com, iommu@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] dma/pool: trivial: add semicolon after label
 attributes

On 2023-08-31 12:59, Chunhui He wrote:
> 
> On Tue, 29 Aug 2023 16:28:05 +0100, Robin Murphy <robin.murphy@....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.