[PATCH V2 0/3] Removing the use of VLAIS from USB Gadget and netfilter

[PATCH V2 0/3] Removing the use of VLAIS from USB Gadget and netfilter

[Behan Webster]

This is a V2 version of the patch set I previously sent out. Since the use of a
new valign.h file spans both the USB and netfilter subsystems, I should have
sent out all 3 patches to both trees at once. Assuming all is well, hopefully
both subsystems can cooperate to get these patches in without causing
conflicts.

The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
precludes the use of compilers which don't implement VLAIS (for instance the
Clang compiler). The LLVMLinux Project is working towards the ability of
providing the Linux kernel developer the choice of using the Clang compiler
toolchain. This is a part of a series of patches which remove the use of VLAIS
from crypto code, dm-crypt, jbd2, libcrc32c, netfilter, and usb gadget. Other
patches to allow Clang to be used to compile the Linux kernel will follow.

Behan Webster (2):
  Helper macros used for replacing the use of VLAIS
  Remove VLAIS usage from gadget code

Jan-Simon Möller (1):
  Remove VLAIS usage from netfilter

 drivers/usb/gadget/f_fs.c   |  106 +++++++++++++++++++++++++------------------
 include/linux/valign.h      |   87 +++++++++++++++++++++++++++++++++++
 net/netfilter/xt_repldata.h |   40 +++++++++-------
 3 files changed, 172 insertions(+), 61 deletions(-)
 create mode 100644 include/linux/valign.h

-- 
1.7.9.5

[PATCH V2 1/3] Helper macros used for replacing the use of VLAIS

[Behan Webster]

The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
precludes the use of compilers which don't implement VLAIS (for instance the
Clang compiler). This new header file contains macros which can be used to
calculate the size and offset of variables in an allocated buffer of memory
taking into account alignment issues.

Signed-off-by: Behan Webster <behanw@converseincode.com>
---
 include/linux/valign.h |   87 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 include/linux/valign.h

diff --git a/include/linux/valign.h b/include/linux/valign.h
new file mode 100644
index 0000000..b39381b
--- /dev/null
+++ b/include/linux/valign.h
@@ -0,0 +1,87 @@
+/*
+ * Variable alignment macros used to break up a larger chunk of memory into
+ * smaller variables. Meant to be used to replace the use of Variable Length
+ * Arrays In Structures (VLAIS)
+ *
+ *  Copyright (C) 2012 Behan Webster <behanw@conversincode.com>
+ */
+
+#ifndef _VALIGN_H_
+#define _VALIGN_H_
+
+/**
+ * truncalign() - Align a memory address by truncation
+ * @num:	Address or size to align
+ * @padwidth:	Number of byte upon which to align
+ *
+ * Truncate an address or size to a particular memory alignment.
+ * Used by truncalign().
+ */
+#define truncalign(num, padwidth) ((long)(num) & ~((padwidth)-1))
+
+/**
+ * padalign() - Align a memory address by padding
+ * @num:	Address or size to align
+ * @padwidth:	Number of byte upon which to align
+ *
+ * Pad out an address or size to a particular memory alignment
+ * Used by paddedsize() and paddedstart().
+ */
+#define padalign(num, padwidth) \
+	truncalign((long)(num) + ((padwidth)-1), padwidth)
+
+/**
+ * paddedsize() - Calculate the size of an chunk of aligned memory
+ * @offset:	Unaligned offset to the start of the chunk size being calculated
+ * @num:	The number of variables in the array of "type" (can be 1)
+ * @type:	The type of variables in the array
+ * @nexttype:	The type of the next variable in the large piece of memory
+ *
+ * Calculate the size that a variable (or array) will take as a part of a
+ * larger piece of memory.  Takes into account a potentially unaligned offset
+ * into the larger piece of allocated memory, the alignment of the variable
+ * type, and the alignement of the type of the variable to be used after that.
+ *
+ * Example: size_t l = paddedsize(1, 2, short, int);
+ *
+ * The example above would give you a padded size of 6 bytes: 2x 16-bit shorts,
+ * starting at 2 bytes into the buffer (the offset of 1 byte being padded out
+ * to 2 bytes) followed by 2 bytes of padding so that the next type (a 32-bit
+ * int) would be 32-bit aligned. looking like this:
+ *
+ *   0: O.SS SS.. iiii
+ *        \-----/ <-- 2 shorts + 2 bytes of padding = size of 6 bytes
+ *
+ * O = The offset
+ * . = Padding bytes
+ * S = 2 shorts
+ * i = int which will theoretically be next
+ */
+#define paddedsize(offset, num, type, nexttype) (padalign((offset) \
+	+ (num) * sizeof(type), __alignof__(nexttype)) - (offset))
+
+/**
+ * paddedstart() - Calculate the start of a chunk of aligned memory
+ * @ptr:	Pointer from which to calculate the start of the chunk
+ * @offset:	Offset from the ptr to the start of the chunk being calculated
+ * @type:	The type of variable in the chunk of memory
+ *
+ * Calculate the start address of a variable based on the offset from an
+ * address, aligned based on the type of the variable specified.
+ *
+ * Example: char *data = kmalloc(size, GFP_KERNEL);
+ *          long *var = paddedstart(data, 12, long);
+ *
+ * The example above on a 64-bit machine would return the equivalent of
+ * &buffer[16] since a long needs to be 8 byte aligned.
+ *
+ *   0: OOOO OOOO OOOO .... LLLL LLLL
+ *                          ^ <-- The start address of the long
+ * O = The offset
+ * . = Padding bytes
+ * L = The long
+ */
+#define paddedstart(ptr, offset, type) \
+	(type *)padalign((long)(ptr)+(offset), __alignof__(type))
+
+#endif
-- 
1.7.9.5

[PATCH V2 2/3] Remove VLAIS usage from gadget code

[Behan Webster]

The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
precludes the use of compilers which don't implement VLAIS (for instance the
Clang compiler). This patch instead calculates offsets into the kmalloc-ed
memory buffer using macros from valign.h.

Signed-off-by: Behan Webster <behanw@converseincode.com>
---
 drivers/usb/gadget/f_fs.c |  106 ++++++++++++++++++++++++++-------------------
 1 file changed, 61 insertions(+), 45 deletions(-)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 64c4ec1..ca6cacc 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -22,6 +22,7 @@
 #include <linux/pagemap.h>
 #include <linux/export.h>
 #include <linux/hid.h>
+#include <linux/valign.h>
 #include <asm/unaligned.h>
 
 #include <linux/usb/composite.h>
@@ -1875,6 +1876,15 @@ error:
 	return ret;
 }
 
+static void __ffs_init_stringtabs(struct usb_gadget_strings **stringtabs,
+	struct usb_gadget_strings *t, unsigned len)
+{
+		do {
+			*stringtabs++ = t++;
+		} while (--len);
+		*stringtabs = NULL;
+}
+
 static int __ffs_data_got_strings(struct ffs_data *ffs,
 				  char *const _data, size_t len)
 {
@@ -1911,31 +1921,26 @@ static int __ffs_data_got_strings(struct ffs_data *ffs,
 
 	/* Allocate everything in one chunk so there's less maintenance. */
 	{
-		struct {
-			struct usb_gadget_strings *stringtabs[lang_count + 1];
-			struct usb_gadget_strings stringtab[lang_count];
-			struct usb_string strings[lang_count*(needed_count+1)];
-		} *d;
-		unsigned i = 0;
-
-		d = kmalloc(sizeof *d, GFP_KERNEL);
+		int strtabslen = paddedsize(0, lang_count+1,
+			struct usb_gadget_strings *, struct usb_gadget_strings);
+		int strtablen = paddedsize(strtabslen, lang_count,
+			struct usb_gadget_strings, struct usb_string);
+		int strlen = paddedsize(strtabslen + strtablen,
+			lang_count*(needed_count+1), struct usb_string, int);
+
+		stringtabs = kmalloc(strtabslen + strtablen + strlen,
+			GFP_KERNEL);
 		if (unlikely(!d)) {
 			kfree(_data);
 			return -ENOMEM;
 		}
 
-		stringtabs = d->stringtabs;
-		t = d->stringtab;
-		i = lang_count;
-		do {
-			*stringtabs++ = t++;
-		} while (--i);
-		*stringtabs = NULL;
-
-		stringtabs = d->stringtabs;
-		t = d->stringtab;
-		s = d->strings;
+		t = paddedstart(stringtabs, strtabslen,
+			struct usb_gadget_strings);
+		s = paddedstart(t, strtablen, struct usb_string);
 		strings = s;
+
+		__ffs_init_stringtabs(stringtabs, t, lang_count);
 	}
 
 	/* For each language */
@@ -2209,17 +2214,23 @@ static int ffs_func_bind(struct usb_configuration *c,
 
 	int ret;
 
-	/* Make it a single chunk, less management later on */
-	struct {
-		struct ffs_ep eps[ffs->eps_count];
-		struct usb_descriptor_header
-			*fs_descs[full ? ffs->fs_descs_count + 1 : 0];
-		struct usb_descriptor_header
-			*hs_descs[high ? ffs->hs_descs_count + 1 : 0];
-		short inums[ffs->interfaces_count];
-		char raw_descs[high ? ffs->raw_descs_length
-				    : ffs->raw_fs_descs_length];
-	} *data;
+	struct ffs_ep *eps;
+	struct usb_descriptor_header **fs_descs;
+	struct usb_descriptor_header **hs_descs;
+	short *inums;
+	char *raw_descs;
+
+	int epslen = paddedsize(0, ffs->eps_count,
+			struct ffs_ep, struct usb_descriptor_header);
+	int fsdlen = paddedsize(epslen, full ? ffs->fs_descs_count + 1 : 0,
+			struct usb_descriptor_header,
+			struct usb_descriptor_header);
+	int hsdlen = paddedsize(fsdlen, high ? ffs->hs_descs_count + 1 : 0,
+			struct usb_descriptor_header, short);
+	int inumlen = paddedsize(hsdlen, ffs->interfaces_count, short, char);
+	int rawlen = paddedsize(inumlen,
+		high ? ffs->raw_descs_length : ffs->raw_fs_descs_length,
+		char, int);
 
 	ENTER();
 
@@ -2227,21 +2238,28 @@ static int ffs_func_bind(struct usb_configuration *c,
 	if (unlikely(!(full | high)))
 		return -ENOTSUPP;
 
-	/* Allocate */
-	data = kmalloc(sizeof *data, GFP_KERNEL);
+	/* Allocate a single chunk, less management later on */
+	eps = kmalloc(epslen+fsdlen+hsdlen+inumlen+rawlen, GFP_KERNEL);
 	if (unlikely(!data))
 		return -ENOMEM;
 
+	/* Calculate start of each variable in the allocated memory */
+	fs_descs = paddedstart(eps, epslen, struct usb_descriptor_header *);
+	hs_descs = paddedstart(fs_descs, fsdlen,
+		struct usb_descriptor_header *);
+	inums = paddedstart(hs_descs, hsdlen, short);
+	raw_descs = paddedstart(inums, inumlen, char);
+
 	/* Zero */
-	memset(data->eps, 0, sizeof data->eps);
-	memcpy(data->raw_descs, ffs->raw_descs + 16, sizeof data->raw_descs);
-	memset(data->inums, 0xff, sizeof data->inums);
+	memset(eps, 0, epslen);
+	memcpy(raw_descs, ffs->raw_descs + 16, rawlen);
+	memset(inums, 0xff, inumlen);
 	for (ret = ffs->eps_count; ret; --ret)
-		data->eps[ret].num = -1;
+		eps[ret].num = -1;
 
 	/* Save pointers */
-	func->eps             = data->eps;
-	func->interfaces_nums = data->inums;
+	func->eps             = eps;
+	func->interfaces_nums = inums;
 
 	/*
 	 * Go through all the endpoint descriptors and allocate
@@ -2249,10 +2267,9 @@ static int ffs_func_bind(struct usb_configuration *c,
 	 * numbers without worrying that it may be described later on.
 	 */
 	if (likely(full)) {
-		func->function.descriptors = data->fs_descs;
+		func->function.descriptors = fs_descs;
 		ret = ffs_do_descs(ffs->fs_descs_count,
-				   data->raw_descs,
-				   sizeof data->raw_descs,
+				   raw_descs, rawlen,
 				   __ffs_func_bind_do_descs, func);
 		if (unlikely(ret < 0))
 			goto error;
@@ -2261,10 +2278,9 @@ static int ffs_func_bind(struct usb_configuration *c,
 	}
 
 	if (likely(high)) {
-		func->function.hs_descriptors = data->hs_descs;
+		func->function.hs_descriptors = hs_descs;
 		ret = ffs_do_descs(ffs->hs_descs_count,
-				   data->raw_descs + ret,
-				   (sizeof data->raw_descs) - ret,
+				   raw_descs + ret, rawlen - ret,
 				   __ffs_func_bind_do_descs, func);
 	}
 
@@ -2275,7 +2291,7 @@ static int ffs_func_bind(struct usb_configuration *c,
 	 */
 	ret = ffs_do_descs(ffs->fs_descs_count +
 			   (high ? ffs->hs_descs_count : 0),
-			   data->raw_descs, sizeof data->raw_descs,
+			   raw_descs, rawlen,
 			   __ffs_func_bind_do_nums, func);
 	if (unlikely(ret < 0))
 		goto error;
-- 
1.7.9.5

Re: [PATCH V2 2/3] Remove VLAIS usage from gadget code

[Felipe Balbi]

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

hi,

On Tue, Oct 30, 2012 at 05:18:56PM -0400, Behan Webster wrote:
> The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
> precludes the use of compilers which don't implement VLAIS (for instance the
> Clang compiler). This patch instead calculates offsets into the kmalloc-ed
> memory buffer using macros from valign.h.
> 
> Signed-off-by: Behan Webster <behanw@converseincode.com>

this won't apply after the current cleanups I applied to gadget code
from Sebastian.

If someone takes this patch, it will generate a series of annoying,
hard-to-figure-out conflicts (at least judging by the looks of
$SUBJECT).

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2 2/3] Remove VLAIS usage from gadget code

[Behan Webster]

On 12-10-31 09:28 AM, Felipe Balbi wrote:
> hi,
>
> On Tue, Oct 30, 2012 at 05:18:56PM -0400, Behan Webster wrote:
>> The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
>> precludes the use of compilers which don't implement VLAIS (for instance the
>> Clang compiler). This patch instead calculates offsets into the kmalloc-ed
>> memory buffer using macros from valign.h.
>>
>> Signed-off-by: Behan Webster <behanw@converseincode.com>
> this won't apply after the current cleanups I applied to gadget code
> from Sebastian.
Makes sense. I'll try it with your repo, and regenerate.

> If someone takes this patch, it will generate a series of annoying,
> hard-to-figure-out conflicts (at least judging by the looks of
> $SUBJECT).
I just tried the patch on your git.kernel.org repo and thankfully there 
is only one hunk which is rejected, and fortunately the reason is 
trivial (descriptors -> fs_descriptors).

Was:
-        func->function.descriptors = data->fs_descs;
+        func->function.descriptors = fs_descs;

Now is:
-        func->function.fs_descriptors = data->fs_descs;
+        func->function.fs_descriptors = fs_descs;

I will regenerate the patch set, but obviously the new gadget patch in 
the V3 patchset will only apply to the USB repo, and not to the 
netfilter repo.

Thanks,

Behan

-- 
Behan Webster
behanw@converseincode.com

Re: [PATCH V2 2/3] Remove VLAIS usage from gadget code

[Felipe Balbi]

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

Hi,

On Wed, Oct 31, 2012 at 11:33:20AM -0400, Behan Webster wrote:
> On 12-10-31 09:28 AM, Felipe Balbi wrote:
> >hi,
> >
> >On Tue, Oct 30, 2012 at 05:18:56PM -0400, Behan Webster wrote:
> >>The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
> >>precludes the use of compilers which don't implement VLAIS (for instance the
> >>Clang compiler). This patch instead calculates offsets into the kmalloc-ed
> >>memory buffer using macros from valign.h.
> >>
> >>Signed-off-by: Behan Webster <behanw@converseincode.com>
> >this won't apply after the current cleanups I applied to gadget code
> >from Sebastian.
> Makes sense. I'll try it with your repo, and regenerate.
> 
> >If someone takes this patch, it will generate a series of annoying,
> >hard-to-figure-out conflicts (at least judging by the looks of
> >$SUBJECT).
> I just tried the patch on your git.kernel.org repo and thankfully
> there is only one hunk which is rejected, and fortunately the reason
> is trivial (descriptors -> fs_descriptors).
> 
> Was:
> -        func->function.descriptors = data->fs_descs;
> +        func->function.descriptors = fs_descs;
> 
> Now is:
> -        func->function.fs_descriptors = data->fs_descs;
> +        func->function.fs_descriptors = fs_descs;
> 
> I will regenerate the patch set, but obviously the new gadget patch
> in the V3 patchset will only apply to the USB repo, and not to the
> netfilter repo.

then we can merge to net tree and handle the conflicts when merging to
Linus, that'd be fine by me as long as people know how to solve the
conflict properly ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2 2/3] Remove VLAIS usage from gadget code

[David Miller]

From: Felipe Balbi <balbi@ti.com>
Date: Thu, 1 Nov 2012 09:21:16 +0200

> then we can merge to net tree and handle the conflicts when merging to
> Linus, that'd be fine by me as long as people know how to solve the
> conflict properly ;-)

Like Herbert with the crypto tree, I'm simply not merging this
absolute crap into my networking tree.

These changes are totally not appropriate.

Re: [PATCH V2 2/3] Remove VLAIS usage from gadget code

[Sebastian Andrzej Siewior]

On Thu, Nov 01, 2012 at 09:21:16AM +0200, Felipe Balbi wrote:
> then we can merge to net tree and handle the conflicts when merging to
> Linus, that'd be fine by me as long as people know how to solve the
> conflict properly ;-)

Felipe please drop this patch. I don't like this VLAIS patch and its
macro magic. I support the goal of compiling the kernel with clang but I
don't think that this is appropriate. Davem has also his problems with
the netfilter part so I am no the only one.
*I* think the gadget part of the kernel is something most people won't
use so it won't show up.
Anyway, please drop this completely I try to think of something sane.

Sebastian

Re: [PATCH V2 2/3] Remove VLAIS usage from gadget code

[Behan Webster]

On 12-12-03 12:57 PM, Sebastian Andrzej Siewior wrote:
> On Thu, Nov 01, 2012 at 09:21:16AM +0200, Felipe Balbi wrote:
>> then we can merge to net tree and handle the conflicts when merging to
>> Linus, that'd be fine by me as long as people know how to solve the
>> conflict properly ;-)
> Felipe please drop this patch. I don't like this VLAIS patch and its
> macro magic. I support the goal of compiling the kernel with clang but I
> don't think that this is appropriate. Davem has also his problems with
> the netfilter part so I am no the only one.
You are definitely not the only one. :)

But that's okay. The idea is to find an alternative that works for 
everyone. It was the first attempt, which didn't work out. <shrug>

> *I* think the gadget part of the kernel is something most people won't
> use so it won't show up.
> Anyway, please drop this completely I try to think of something sane.
He actually didn't take the patch. He was asking the netfilter team to 
take it, and they didn't want to. Fair enough.

However, in order to approximate what gcc is doing in code, obviously 
some math is required. The thought was that macros would hide the worst 
of it, trying not to obfuscate what was actually being done.

One of the project members came up with this alternative. How about 
something like this? Less math, though more string pasting. When 
compiled, the unused variables get optimized away. Otherwise the memory 
packing is identical to using VLAIS in gcc.

#define vla_struct(structname) size_t structname##__##next = 0
#define vla_struct_size(structname) structname##__##next

#define vla_item(structname, type, name, n) \
         type * structname##_##name; \
         size_t structname##_##name##__##pad = \
                 (structname##__##next & (__alignof__(type)-1)); \
         size_t structname##_##name##__##offset = \
                 structname##__##next + structname##_##name##__##pad; \
         size_t structname##_##name##__##sz = n * sizeof(type); \
         structname##__##next = structname##__##next + \
                 structname##_##name##__##pad + 
structname##_##name##__##sz;

#define vla_ptr(ptr,structname,name) structname##_##name = \
(typeof(structname##_##name))&ptr[structname##_##name##__##offset]


Then you can do something like this that looks vaguely struct-like:

         vla_struct(foo);
                 vla_item(foo, char,  vara, 1);
                 vla_item(foo, short, varb, 10);
                 vla_item(foo, int,   varc, 5);
                 vla_item(foo, long,  vard, 3);
         size_t total = vla_struct_size(foo);
         char buffer[total];

         vla_ptr(buffer, foo, varc);
         foo_varc = 1;

I've been profiling some sample code around this implementation 
comparing it between compilers, and it approximates the code size and 
speed of using VLAIS in gcc (especially with -O2). It actually performs 
better than the previously proposed macros.

But certainly if anyone has a solution which works for everyone, then 
I'm more than happy to adopt it. The LLVM community has made quite a few 
changes in order to help get Linux working with clang. However, VLAIS is 
not something they are willing to accept (for various reasons). There 
are other patches to LLVM that are still working their way upstream that 
are required to be able to compile Linux as well.

As far as I've been told (though I have been unable to verify), Variable 
Length Arrays In Structs (VLAIS) is a feature which made it into gcc 
along with nested functions by the work on the Ada frontend. By 
contrast, the C99 standard specifies Variable Length Arrays (outside of 
structs) and Flexible Array Members (essentially zero-length arrays at 
the end of a struct), and as such both are supported by gcc and clang.

The LLVMLinux project is a meta-project between the LLVM and Linux 
communities trying to get the toolchain from one working with the code 
from the other.  The LLVMLinux project isn't trying to replace, nor 
break gcc (in fact all the project's kernel patches are tested with gcc 
as well). The idea is to bring another toolchain and set of tools to the 
table and the kernel community.

Behan

-- 
Behan Webster
behanw@converseincode.com

Re: [PATCH V2 2/3] Remove VLAIS usage from gadget code

[Sebastian Andrzej Siewior]

On Mon, Dec 03, 2012 at 07:57:33PM +0100, Behan Webster wrote:
> However, in order to approximate what gcc is doing in code, obviously
> some math is required. The thought was that macros would hide the
> worst of it, trying not to obfuscate what was actually being done.

Why hide? The thing that is done here is setting up pointers and keep this
struct as a container. It is never used again, just freed. Therefore I would
suggest to remove and do something different.

> One of the project members came up with this alternative. How about
> something like this? Less math, though more string pasting. When
> compiled, the unused variables get optimized away. Otherwise the
> memory packing is identical to using VLAIS in gcc.
> 
> #define vla_struct(structname) size_t structname##__##next = 0
> #define vla_struct_size(structname) structname##__##next
> 
> #define vla_item(structname, type, name, n) \
>         type * structname##_##name; \
>         size_t structname##_##name##__##pad = \
>                 (structname##__##next & (__alignof__(type)-1)); \
>         size_t structname##_##name##__##offset = \
>                 structname##__##next + structname##_##name##__##pad; \
>         size_t structname##_##name##__##sz = n * sizeof(type); \
>         structname##__##next = structname##__##next + \
>                 structname##_##name##__##pad +
> structname##_##name##__##sz;
> 
> #define vla_ptr(ptr,structname,name) structname##_##name = \
> (typeof(structname##_##name))&ptr[structname##_##name##__##offset]
> 
> 
> Then you can do something like this that looks vaguely struct-like:
> 
>         vla_struct(foo);
>                 vla_item(foo, char,  vara, 1);
>                 vla_item(foo, short, varb, 10);
>                 vla_item(foo, int,   varc, 5);
>                 vla_item(foo, long,  vard, 3);
>         size_t total = vla_struct_size(foo);
>         char buffer[total];
> 
>         vla_ptr(buffer, foo, varc);
>         foo_varc = 1;

I prefer to try to rewritte the code in the gadget in a different manner
before using macro magic. I guess most people around here think that extensive
usage of macros equals giving a gun to a chimpanzee. It may work for a while
and may even look cute in the eye of the gun sponsor. However once it fires…

> I've been profiling some sample code around this implementation
> comparing it between compilers, and it approximates the code size and
> speed of using VLAIS in gcc (especially with -O2). It actually
> performs better than the previously proposed macros.

I'm not concerned about speed here. This is an one time setup.

> But certainly if anyone has a solution which works for everyone, then
> I'm more than happy to adopt it. The LLVM community has made quite a
> few changes in order to help get Linux working with clang. However,

That is nice to hear. Besides gcc there is the icc.

> VLAIS is not something they are willing to accept (for various
> reasons). There are other patches to LLVM that are still working
Is this not described in C99 6.7.2.1p16?

> their way upstream that are required to be able to compile Linux as
> well.

I hope the other are "simple" to get in :)

> 
> Behan
> 

Sebastian

Re: [PATCH V2 2/3] Remove VLAIS usage from gadget code

[Behan Webster]

On 12-12-04 11:24 PM, Sebastian Andrzej Siewior wrote:
> On Mon, Dec 03, 2012 at 07:57:33PM +0100, Behan Webster wrote:
>> However, in order to approximate what gcc is doing in code, obviously
>> some math is required. The thought was that macros would hide the
>> worst of it, trying not to obfuscate what was actually being done.
> Why hide? The thing that is done here is setting up pointers and keep this
> struct as a container. It is never used again, just freed. Therefore I would
> suggest to remove and do something different.
Fair enough. That works too.

>> I've been profiling some sample code around this implementation
>> comparing it between compilers, and it approximates the code size and
>> speed of using VLAIS in gcc (especially with -O2). It actually
>> performs better than the previously proposed macros.
> I'm not concerned about speed here. This is an one time setup.
Which is all good in the case of the gadget code.

However VLAIS is used in other places too where code size and speed are 
very important. The proposal was for something which might be used for 
more than just the gadget code. VLAIS is used in only a handful of 
places in the kernel. They just happen to be in important parts of the 
kernel which was why the approach was to change as little as necessary.

>> But certainly if anyone has a solution which works for everyone, then
>> I'm more than happy to adopt it. The LLVM community has made quite a
>> few changes in order to help get Linux working with clang. However,
> That is nice to hear. Besides gcc there is the icc.
Lots of people have brought up icc. Although in the same breath most 
also say nobody is using it anymore. However, I have no idea whether 
that is true. The difference is that clang is widely available in most 
distros; icc is not.

>> VLAIS is not something they are willing to accept (for various
>> reasons). There are other patches to LLVM that are still working
> Is this not described in C99 6.7.2.1p16?
You're thinking of "Flexible Array Members". Essentially the last 
element in a structure can be the equivalent of a zero length array; 
something which both gcc and clang support today.

 From C99 6.7.2.1p16:
> 16 As a special case, the last element of a structure with more than 
> one named member may
> have an incomplete array type; this is called a flexible array member. 
> In most situations,
> the flexible array member is ignored. In particular, the size of the 
> structure is as if the
> flexible array member were omitted except that it may have more 
> trailing padding than
> the omission would imply. Howev er, when a . (or ->) operator has a 
> left operand that is
> (a pointer to) a structure with a flexible array member and the right 
> operand names that
> member, it behaves as if that member were replaced with the longest 
> array (with the same
> element type) that would not make the structure larger than the object 
> being accessed; the
> offset of the array shall remain that of the flexible array member, 
> even if this would differ
> from that of the replacement array. If this array would have no 
> elements, it behaves as if
> it had one element but the behavior is undefined if any attempt is made 
> to access that
> element or to generate a pointer one past it.

However if we're considering the C99 standard, it also explicitly 
disallows the use of VLAs in structs (VLAIS).

 From C99 6.7.2.1 p2:
> 2 A structure or union shall not contain a member with incomplete or 
> function type (hence,
> a structure shall not contain an instance of itself, but may contain a 
> pointer to an instance
> of itself), except that the last member of a structure with more than 
> one named member
> may have incomplete array type; such a structure (and any union 
> containing, possibly
> recursively, a member that is such a structure) shall not be a member 
> of a structure or an
> element of an array.
 From C99 6.7.5.2 p4:
> 4 If the size is not present, the array type is an incomplete type. <snip>
Essentially all arrays in a structure must not be variable length except 
the last member of the array which can be a zero length array (which can 
be used to approximate a VLA at the end). It also says that a struct 
which uses a flexible array member can't be used inside another struct.

>> their way upstream that are required to be able to compile Linux as
>> well.
> I hope the other are "simple" to get in :)
I hope so too. But like the Linux kernel community, the LLVM community 
have standards and procedures for getting code accepted which take time.

Thanks,

Behan

-- 
Behan Webster
behanw@converseincode.com

Re: [llvmlinux] [PATCH V2 2/3] Remove VLAIS usage from gadget code

[Bryce Lelbach]

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

On 2012.12.04 23.24, Sebastian Andrzej Siewior wrote:
> > VLAIS is not something they are willing to accept (for various
> > reasons). There are other patches to LLVM that are still working
> Is this not described in C99 6.7.2.1p16?

No, this is not the case - flexible array members are a very different beast,
which are in-and-of-themselves of debatable merit. C99 6.7.2.1p8 explicitly
states that VLAIS are illegal. The same language is present in C11 (same
section):

	A member of a structure or union may have any object type other than a variably
	modified type.

In C99 6.7.5.1p3, the definition of a variably modified type is given:

	.. [snip] ... If the nested sequence of declarators in a full declarator
	contains a variable length array type, the type specified by the full
	declarator is said to be variably modified.

It would require incredible, earth-moving acts for VLAIS to become a part of the
ISO/IEC standard for the C programming language.

P.S. As a general note, while I do agree that it is preferable to rewrite code
using VLAIS instead of utilizing a macro, I would ask that some reconsideration
be given, unless there are Linux kernel developers who are willing and able
to implement the necessary rewrites. The LLVMLinux project does not wish to
decrease the code quality of the Linux kernel, and certainly we do not intend to
cause any GCC breakage.

However, I think that rewrites to all the relevant VLAIS code would be quite
time consuming for the LLVMLinux team, and the proposed macro does not appear
to me to present any particular evil, other than by nature of being a macro.

If faced between the choice of either accepting this VLAIS patch, or not being
able to compile the Linux kernel with standard-compliant, largely GCC-compatible
compilers such as Clang and icc, I would think that accepting the VLAIS patch
would be preferable.

(note also that I have only come in on the end of this thread, and I may have
missed part of the context).

-- 
Bryce Adelstein-Lelbach aka wash
STE||AR Group, Center for Computation and Technology, LSU
--
860-808-7497 - Cell
225-578-6182 - Work (no voicemail)
--
stellar.cct.lsu.edu
boost-spirit.com
llvm.linuxfoundation.org
--

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[PATCH V2 3/3] Remove VLAIS usage from netfilter

[Behan Webster]

From: Jan-Simon Möller <dl9pf@gmx.de>

The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
precludes the use of compilers which don't implement VLAIS (for instance the
Clang compiler). This patch instead calculates offsets into the kmalloc-ed
memory buffer using macros from valign.h.

Patch from series at
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120507/142707.html
by PaX Team.

Signed-off-by: Jan-Simon Möller <dl9pf@gmx.de>
Cc: pageexec@freemail.hu
[Modified to use macros from valign.h]
Signed-off-by: Behan Webster <behanw@converseincode.com>
---
 net/netfilter/xt_repldata.h |   40 ++++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/net/netfilter/xt_repldata.h b/net/netfilter/xt_repldata.h
index 6efe4e5..d2b4232 100644
--- a/net/netfilter/xt_repldata.h
+++ b/net/netfilter/xt_repldata.h
@@ -5,31 +5,39 @@
  * they serve as the hanging-off data accessed through repl.data[].
  */
 
+#include <linux/valign.h>
+
 #define xt_alloc_initial_table(type, typ2) ({ \
 	unsigned int hook_mask = info->valid_hooks; \
 	unsigned int nhooks = hweight32(hook_mask); \
 	unsigned int bytes = 0, hooknum = 0, i = 0; \
-	struct { \
-		struct type##_replace repl; \
-		struct type##_standard entries[nhooks]; \
-		struct type##_error term; \
-	} *tbl = kzalloc(sizeof(*tbl), GFP_KERNEL); \
-	if (tbl == NULL) \
+	int replsize = paddedsize(0, 1, \
+		struct type##_replace, struct type##_standard); \
+	int entsize = paddedsize(replsize, nhooks, \
+		struct type##_standard, struct type##_error); \
+	int termsize = paddedsize(replsize+entsize, 1, \
+		struct type##_error, int); \
+	struct type##_replace *repl = kzalloc(replsize+entsize+termsize, \
+		GFP_KERNEL); \
+	if (repl == NULL) \
 		return NULL; \
-	strncpy(tbl->repl.name, info->name, sizeof(tbl->repl.name)); \
-	tbl->term = (struct type##_error)typ2##_ERROR_INIT;  \
-	tbl->repl.valid_hooks = hook_mask; \
-	tbl->repl.num_entries = nhooks + 1; \
-	tbl->repl.size = nhooks * sizeof(struct type##_standard) + \
-	                 sizeof(struct type##_error); \
+	struct type##_standard *entries = paddedstart(repl, replsize, \
+		struct type##_standard); \
+	struct type##_error *term = paddedstart(entries, entsize, \
+		struct type##_error); \
+	strncpy(repl->name, info->name, sizeof(repl->name)); \
+	*term = (struct type##_error)typ2##_ERROR_INIT;  \
+	repl->valid_hooks = hook_mask; \
+	repl->num_entries = nhooks + 1; \
+	repl->size = entsize+termsize; \
 	for (; hook_mask != 0; hook_mask >>= 1, ++hooknum) { \
 		if (!(hook_mask & 1)) \
 			continue; \
-		tbl->repl.hook_entry[hooknum] = bytes; \
-		tbl->repl.underflow[hooknum]  = bytes; \
-		tbl->entries[i++] = (struct type##_standard) \
+		repl->hook_entry[hooknum] = bytes; \
+		repl->underflow[hooknum]  = bytes; \
+		entries[i++] = (struct type##_standard) \
 			typ2##_STANDARD_INIT(NF_ACCEPT); \
 		bytes += sizeof(struct type##_standard); \
 	} \
-	tbl; \
+	repl; \
 })
-- 
1.7.9.5

Re: [PATCH V2 0/3] Removing the use of VLAIS from USB Gadget and netfilter

[David Miller]

I'm not pulling this crap into my tree to deal with limitations
of a non-gcc compiler.