PROBLEM: compilation issue, Incorrect C usage in drivers/block/null_blk.c causes kernel compilation failure with Intel c++ compiler

PROBLEM: compilation issue, Incorrect C usage in drivers/block/null_blk.c causes kernel compilation failure with Intel c++ compiler

[Blower, Melanie]

[1.]  Incorrect C usage in drivers/block/null_blk.c causes kernel compilation failure with Intel c++ compiler
[2.] Full description of the problem/report:
Using icc,
    drivers/block/null_blk.c(569): error: variable "null_lnvm_dev_ops" was declared with a never-completed type
    static struct nvm_dev_ops null_lnvm_dev_ops; 

Clark Nelson, one of Intel's C++ language lawyers, explains why this declaration is illegal:

 Discussion:
 Here is the problematic declaration, which appears near line 585 of file drivers/block/null_blk.c:
 
      static struct nvm_dev_ops null_lnvm_dev_ops;
 
 This clearly violates 6.9.2 paragraph 3 of the C standard:
 
 If the declaration of an identifier for an object is a tentative definition and has internal linkage, the declared type shall not be an  incomplete type.
 
 The declaration is a tentative definition because it has no  initializer, it has internal linkage because the static keyword is  used, and yet the type is definitely an incomplete type.
 
 In general, the problem with such a declaration is that the compiler is expected to allocate memory for it in some data section, but the  compiler is given no idea how much memory to allocate.
 
 As it turns out, the only way this variable is used is to take its  address and pass it to an inline function, which doesn't use the  corresponding pointer parameter at all. So after inlining and dead  code elimination, no reference to the variable survives, so it doesn't  need to be allocated after all, so it doesn't matter that the size of the allocation isn't known.
 
 When optimization isn't used, and the variable isn't discovered to be  unnecessary, GCC also reports an error for this declaration.
 
 However, it's fairly obvious that GCC wasn't carefully and  consistently designed to allow this sort of thing. If the same  declaration appears in a block scope, without the static keyword, GCC  reports an error regardless of the optimization level, even if the  variable is never referenced at all, even if the declaration appears  in a static function that is never used. The logic by which the original static declaration is accepted would suggest that the auto declaration should also be accepted, but it isn't.
 
 The simple fix would be to add an empty member list to the declaration:
 
 static struct nvm_dev_ops {} null_lnvm_dev_ops;
 
 That would still not conform to the C standard, which doesn't allow an empty member list. But support for an empty member list is much more  stable and predictable than support for optimizing out unused invalid  variable declarations.


[3.] Keywords : kernel
[4.] Kernel version (from /proc/version): 
 We're building linux-4.4-rc8.tar.xz with the Intel compiler
Here's host information where the Kernel build occurs:
Linux version 3.10.0-229.el7.x86_64 (mockbuild@x86-035.build.eng.bos.redhat.com) (gcc version 4.8.3 20140911 (Red Hat 4.8.3-7) (GCC) ) #1 SMP Thu Jan 29 18:37:38 EST 2015
[5.] Output of Oops.. message (if applicable) with symbolic information 
     resolved (see Documentation/oops-tracing.txt)
n/a
[6.] A small shell script or example program which triggers the
     problem (if possible)
Compile kernel linux-4.4-rc8.tar.xz with Intel C++ compiler for Linux version 16.0 update 1
[7.] Environment
I'm not going to include the environment details since they're irrelevant.
[7.1.] Software (add the output of the ver_linux script here)
[7.2.] Processor information (from /proc/cpuinfo):
[7.3.] Module information (from /proc/modules):
[7.4.] Loaded driver and hardware information (/proc/ioports, /proc/iomem)
[7.5.] PCI information ('lspci -vvv' as root)
[7.6.] SCSI information (from /proc/scsi/scsi)
[7.7.] Other information that might be relevant to the problem
       (please look in /proc and include all information that you
       think to be relevant):
[X.] Other notes, patches, fixes, workarounds:

Please cc: me on replies to this message. Thanks and regards, Melanie Blower (Intel C++ compiler team)

Re: PROBLEM: compilation issue, Incorrect C usage in drivers/block/null_blk.c causes kernel compilation failure with Intel c++ compiler

[Randy Dunlap]

On 01/13/16 11:22, Blower, Melanie wrote:
> [1.]  Incorrect C usage in drivers/block/null_blk.c causes kernel compilation failure with Intel c++ compiler
> [2.] Full description of the problem/report:
> Using icc,
>     drivers/block/null_blk.c(569): error: variable "null_lnvm_dev_ops" was declared with a never-completed type
>     static struct nvm_dev_ops null_lnvm_dev_ops; 
> 
> Clark Nelson, one of Intel's C++ language lawyers, explains why this declaration is illegal:
> 
>  Discussion:
>  Here is the problematic declaration, which appears near line 585 of file drivers/block/null_blk.c:
>  
>       static struct nvm_dev_ops null_lnvm_dev_ops;


or with gcc when building linux-next-20160111:

../drivers/block/null_blk.c:569:27: error: storage size of €˜'null_lnvm_dev_ops€'™ isn'€™t known
 static struct nvm_dev_ops null_lnvm_dev_ops;
                           ^


>  This clearly violates 6.9.2 paragraph 3 of the C standard:
>  
>  If the declaration of an identifier for an object is a tentative definition and has internal linkage, the declared type shall not be an  incomplete type.
>  
>  The declaration is a tentative definition because it has no  initializer, it has internal linkage because the static keyword is  used, and yet the type is definitely an incomplete type.
>  
>  In general, the problem with such a declaration is that the compiler is expected to allocate memory for it in some data section, but the  compiler is given no idea how much memory to allocate.
>  
>  As it turns out, the only way this variable is used is to take its  address and pass it to an inline function, which doesn't use the  corresponding pointer parameter at all. So after inlining and dead  code elimination, no reference to the variable survives, so it doesn't  need to be allocated after all, so it doesn't matter that the size of the allocation isn't known.
>  
>  When optimization isn't used, and the variable isn't discovered to be  unnecessary, GCC also reports an error for this declaration.
>  
>  However, it's fairly obvious that GCC wasn't carefully and  consistently designed to allow this sort of thing. If the same  declaration appears in a block scope, without the static keyword, GCC  reports an error regardless of the optimization level, even if the  variable is never referenced at all, even if the declaration appears  in a static function that is never used. The logic by which the original static declaration is accepted would suggest that the auto declaration should also be accepted, but it isn't.
>  
>  The simple fix would be to add an empty member list to the declaration:
>  
>  static struct nvm_dev_ops {} null_lnvm_dev_ops;
>  
>  That would still not conform to the C standard, which doesn't allow an empty member list. But support for an empty member list is much more  stable and predictable than support for optimizing out unused invalid  variable declarations.
> 
> 
> [3.] Keywords : kernel
> [4.] Kernel version (from /proc/version): 
>  We're building linux-4.4-rc8.tar.xz with the Intel compiler
> Here's host information where the Kernel build occurs:
> Linux version 3.10.0-229.el7.x86_64 (mockbuild@x86-035.build.eng.bos.redhat.com) (gcc version 4.8.3 20140911 (Red Hat 4.8.3-7) (GCC) ) #1 SMP Thu Jan 29 18:37:38 EST 2015
> [5.] Output of Oops.. message (if applicable) with symbolic information 
>      resolved (see Documentation/oops-tracing.txt)
> n/a
> [6.] A small shell script or example program which triggers the
>      problem (if possible)
> Compile kernel linux-4.4-rc8.tar.xz with Intel C++ compiler for Linux version 16.0 update 1
> [7.] Environment
> I'm not going to include the environment details since they're irrelevant.
> [7.1.] Software (add the output of the ver_linux script here)
> [7.2.] Processor information (from /proc/cpuinfo):
> [7.3.] Module information (from /proc/modules):
> [7.4.] Loaded driver and hardware information (/proc/ioports, /proc/iomem)
> [7.5.] PCI information ('lspci -vvv' as root)
> [7.6.] SCSI information (from /proc/scsi/scsi)
> [7.7.] Other information that might be relevant to the problem
>        (please look in /proc and include all information that you
>        think to be relevant):
> [X.] Other notes, patches, fixes, workarounds:
> 
> Please cc: me on replies to this message. Thanks and regards, Melanie Blower (Intel C++ compiler team)
> 


-- 
~Randy

Re: PROBLEM: compilation issue, Incorrect C usage in drivers/block/null_blk.c causes kernel compilation failure with Intel c++ compiler

[Randy Dunlap]

[add Jens Axboe]


On 01/13/16 11:35, Randy Dunlap wrote:
> On 01/13/16 11:22, Blower, Melanie wrote:
>> [1.]  Incorrect C usage in drivers/block/null_blk.c causes kernel compilation failure with Intel c++ compiler
>> [2.] Full description of the problem/report:
>> Using icc,
>>     drivers/block/null_blk.c(569): error: variable "null_lnvm_dev_ops" was declared with a never-completed type
>>     static struct nvm_dev_ops null_lnvm_dev_ops; 
>>
>> Clark Nelson, one of Intel's C++ language lawyers, explains why this declaration is illegal:
>>
>>  Discussion:
>>  Here is the problematic declaration, which appears near line 585 of file drivers/block/null_blk.c:
>>  
>>       static struct nvm_dev_ops null_lnvm_dev_ops;
> 
> 
> or with gcc when building linux-next-20160111:
> 
> ../drivers/block/null_blk.c:569:27: error: storage size of €˜'null_lnvm_dev_ops€'™ isn'€™t known
>  static struct nvm_dev_ops null_lnvm_dev_ops;
>                            ^
> 
> 
>>  This clearly violates 6.9.2 paragraph 3 of the C standard:
>>  
>>  If the declaration of an identifier for an object is a tentative definition and has internal linkage, the declared type shall not be an  incomplete type.
>>  
>>  The declaration is a tentative definition because it has no  initializer, it has internal linkage because the static keyword is  used, and yet the type is definitely an incomplete type.
>>  
>>  In general, the problem with such a declaration is that the compiler is expected to allocate memory for it in some data section, but the  compiler is given no idea how much memory to allocate.
>>  
>>  As it turns out, the only way this variable is used is to take its  address and pass it to an inline function, which doesn't use the  corresponding pointer parameter at all. So after inlining and dead  code elimination, no reference to the variable survives, so it doesn't  need to be allocated after all, so it doesn't matter that the size of the allocation isn't known.
>>  
>>  When optimization isn't used, and the variable isn't discovered to be  unnecessary, GCC also reports an error for this declaration.
>>  
>>  However, it's fairly obvious that GCC wasn't carefully and  consistently designed to allow this sort of thing. If the same  declaration appears in a block scope, without the static keyword, GCC  reports an error regardless of the optimization level, even if the  variable is never referenced at all, even if the declaration appears  in a static function that is never used. The logic by which the original static declaration is accepted would suggest that the auto declaration should also be accepted, but it isn't.
>>  
>>  The simple fix would be to add an empty member list to the declaration:
>>  
>>  static struct nvm_dev_ops {} null_lnvm_dev_ops;
>>  
>>  That would still not conform to the C standard, which doesn't allow an empty member list. But support for an empty member list is much more  stable and predictable than support for optimizing out unused invalid  variable declarations.
>>
>>
>> [3.] Keywords : kernel
>> [4.] Kernel version (from /proc/version): 
>>  We're building linux-4.4-rc8.tar.xz with the Intel compiler
>> Here's host information where the Kernel build occurs:
>> Linux version 3.10.0-229.el7.x86_64 (mockbuild@x86-035.build.eng.bos.redhat.com) (gcc version 4.8.3 20140911 (Red Hat 4.8.3-7) (GCC) ) #1 SMP Thu Jan 29 18:37:38 EST 2015
>> [5.] Output of Oops.. message (if applicable) with symbolic information 
>>      resolved (see Documentation/oops-tracing.txt)
>> n/a
>> [6.] A small shell script or example program which triggers the
>>      problem (if possible)
>> Compile kernel linux-4.4-rc8.tar.xz with Intel C++ compiler for Linux version 16.0 update 1
>> [7.] Environment
>> I'm not going to include the environment details since they're irrelevant.
>> [7.1.] Software (add the output of the ver_linux script here)
>> [7.2.] Processor information (from /proc/cpuinfo):
>> [7.3.] Module information (from /proc/modules):
>> [7.4.] Loaded driver and hardware information (/proc/ioports, /proc/iomem)
>> [7.5.] PCI information ('lspci -vvv' as root)
>> [7.6.] SCSI information (from /proc/scsi/scsi)
>> [7.7.] Other information that might be relevant to the problem
>>        (please look in /proc and include all information that you
>>        think to be relevant):
>> [X.] Other notes, patches, fixes, workarounds:
>>
>> Please cc: me on replies to this message. Thanks and regards, Melanie Blower (Intel C++ compiler team)
>>
> 
> 


-- 
~Randy

Re: PROBLEM: compilation issue, Incorrect C usage in drivers/block/null_blk.c causes kernel compilation failure with Intel c++ compiler

[Jens Axboe]

[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]

On 01/13/2016 12:37 PM, Randy Dunlap wrote:
> [add Jens Axboe]
>
>
> On 01/13/16 11:35, Randy Dunlap wrote:
>> On 01/13/16 11:22, Blower, Melanie wrote:
>>> [1.]  Incorrect C usage in drivers/block/null_blk.c causes kernel compilation failure with Intel c++ compiler
>>> [2.] Full description of the problem/report:
>>> Using icc,
>>>      drivers/block/null_blk.c(569): error: variable "null_lnvm_dev_ops" was declared with a never-completed type
>>>      static struct nvm_dev_ops null_lnvm_dev_ops;
>>>
>>> Clark Nelson, one of Intel's C++ language lawyers, explains why this declaration is illegal:
>>>
>>>   Discussion:
>>>   Here is the problematic declaration, which appears near line 585 of file drivers/block/null_blk.c:
>>>
>>>        static struct nvm_dev_ops null_lnvm_dev_ops;

So that's a very verbose way of saying that the structure is undefined 
if CONFIG_NVM isn't set. I agree, that's crap code, doesn't make any 
sense. Surprised gcc doesn't complain about it.

Something like the attached should fix it, making enough visible with 
CONFIG_NVM that we can declare an empty ops type.

-- 
Jens Axboe


[-- Attachment #2: nvm-ops.patch --]
[-- Type: text/x-patch, Size: 4194 bytes --]

diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
index 3db5552b17d5..b0965c110b62 100644
--- a/include/linux/lightnvm.h
+++ b/include/linux/lightnvm.h
@@ -11,6 +11,65 @@ enum {
 	NVM_IOTYPE_GC = 1,
 };
 
+struct nvm_id;
+struct nvm_rq;
+
+#define NVM_BLK_BITS (16)
+#define NVM_PG_BITS  (16)
+#define NVM_SEC_BITS (8)
+#define NVM_PL_BITS  (8)
+#define NVM_LUN_BITS (8)
+#define NVM_CH_BITS  (8)
+
+struct ppa_addr {
+	/* Generic structure for all addresses */
+	union {
+		struct {
+			u64 blk		: NVM_BLK_BITS;
+			u64 pg		: NVM_PG_BITS;
+			u64 sec		: NVM_SEC_BITS;
+			u64 pl		: NVM_PL_BITS;
+			u64 lun		: NVM_LUN_BITS;
+			u64 ch		: NVM_CH_BITS;
+		} g;
+
+		u64 ppa;
+	};
+};
+
+typedef int (nvm_l2p_update_fn)(u64, u32, __le64 *, void *);
+typedef int (nvm_bb_update_fn)(struct ppa_addr, int, u8 *, void *);
+typedef int (nvm_id_fn)(struct request_queue *, struct nvm_id *);
+typedef int (nvm_get_l2p_tbl_fn)(struct request_queue *, u64, u32,
+				nvm_l2p_update_fn *, void *);
+typedef int (nvm_op_bb_tbl_fn)(struct request_queue *, struct ppa_addr, int,
+				nvm_bb_update_fn *, void *);
+typedef int (nvm_op_set_bb_fn)(struct request_queue *, struct nvm_rq *, int);
+typedef int (nvm_submit_io_fn)(struct request_queue *, struct nvm_rq *);
+typedef int (nvm_erase_blk_fn)(struct request_queue *, struct nvm_rq *);
+typedef void *(nvm_create_dma_pool_fn)(struct request_queue *, char *);
+typedef void (nvm_destroy_dma_pool_fn)(void *);
+typedef void *(nvm_dev_dma_alloc_fn)(struct request_queue *, void *, gfp_t,
+								dma_addr_t *);
+typedef void (nvm_dev_dma_free_fn)(void *, void*, dma_addr_t);
+
+struct nvm_dev_ops {
+	nvm_id_fn		*identity;
+	nvm_get_l2p_tbl_fn	*get_l2p_tbl;
+	nvm_op_bb_tbl_fn	*get_bb_tbl;
+	nvm_op_set_bb_fn	*set_bb_tbl;
+
+	nvm_submit_io_fn	*submit_io;
+	nvm_erase_blk_fn	*erase_block;
+
+	nvm_create_dma_pool_fn	*create_dma_pool;
+	nvm_destroy_dma_pool_fn	*destroy_dma_pool;
+	nvm_dev_dma_alloc_fn	*dev_dma_alloc;
+	nvm_dev_dma_free_fn	*dev_dma_free;
+
+	unsigned int		max_phys_sect;
+};
+
 #ifdef CONFIG_NVM
 
 #include <linux/blkdev.h>
@@ -118,29 +177,6 @@ struct nvm_tgt_instance {
 #define NVM_VERSION_MINOR 0
 #define NVM_VERSION_PATCH 0
 
-#define NVM_BLK_BITS (16)
-#define NVM_PG_BITS  (16)
-#define NVM_SEC_BITS (8)
-#define NVM_PL_BITS  (8)
-#define NVM_LUN_BITS (8)
-#define NVM_CH_BITS  (8)
-
-struct ppa_addr {
-	/* Generic structure for all addresses */
-	union {
-		struct {
-			u64 blk		: NVM_BLK_BITS;
-			u64 pg		: NVM_PG_BITS;
-			u64 sec		: NVM_SEC_BITS;
-			u64 pl		: NVM_PL_BITS;
-			u64 lun		: NVM_LUN_BITS;
-			u64 ch		: NVM_CH_BITS;
-		} g;
-
-		u64 ppa;
-	};
-};
-
 struct nvm_rq {
 	struct nvm_tgt_instance *ins;
 	struct nvm_dev *dev;
@@ -174,39 +210,6 @@ static inline void *nvm_rq_to_pdu(struct nvm_rq *rqdata)
 
 struct nvm_block;
 
-typedef int (nvm_l2p_update_fn)(u64, u32, __le64 *, void *);
-typedef int (nvm_bb_update_fn)(struct ppa_addr, int, u8 *, void *);
-typedef int (nvm_id_fn)(struct request_queue *, struct nvm_id *);
-typedef int (nvm_get_l2p_tbl_fn)(struct request_queue *, u64, u32,
-				nvm_l2p_update_fn *, void *);
-typedef int (nvm_op_bb_tbl_fn)(struct request_queue *, struct ppa_addr, int,
-				nvm_bb_update_fn *, void *);
-typedef int (nvm_op_set_bb_fn)(struct request_queue *, struct nvm_rq *, int);
-typedef int (nvm_submit_io_fn)(struct request_queue *, struct nvm_rq *);
-typedef int (nvm_erase_blk_fn)(struct request_queue *, struct nvm_rq *);
-typedef void *(nvm_create_dma_pool_fn)(struct request_queue *, char *);
-typedef void (nvm_destroy_dma_pool_fn)(void *);
-typedef void *(nvm_dev_dma_alloc_fn)(struct request_queue *, void *, gfp_t,
-								dma_addr_t *);
-typedef void (nvm_dev_dma_free_fn)(void *, void*, dma_addr_t);
-
-struct nvm_dev_ops {
-	nvm_id_fn		*identity;
-	nvm_get_l2p_tbl_fn	*get_l2p_tbl;
-	nvm_op_bb_tbl_fn	*get_bb_tbl;
-	nvm_op_set_bb_fn	*set_bb_tbl;
-
-	nvm_submit_io_fn	*submit_io;
-	nvm_erase_blk_fn	*erase_block;
-
-	nvm_create_dma_pool_fn	*create_dma_pool;
-	nvm_destroy_dma_pool_fn	*destroy_dma_pool;
-	nvm_dev_dma_alloc_fn	*dev_dma_alloc;
-	nvm_dev_dma_free_fn	*dev_dma_free;
-
-	unsigned int		max_phys_sect;
-};
-
 struct nvm_lun {
 	int id;