Commit 4c8bd7ee ("Do not truncate to 'int' in ALIGN() macro.") was
merged to fix the case of code like the following:
unsigned long addr;
unsigned int alignment;
addr = ALIGN(addr, alignment);
The original ALIGN macro calculated a mask as ~(alignment - 1), and
when alignment is just an int, this creates an int mask. If alignment
is also unsigned, then this mask will not be sign extended when
promoted to a long, which leads to the code above chopping off the top
half of addr when long is 64 bits.
However, the changed ALIGN macro, which computes the mask as
~(alignment - 1UL) actually breaks code like the following when long
is 32 bits:
u64 addr;
int alignment;
addr = ALIGN(addr, alignment);
The reason this breaks is pretty much the same as the original bug
that the change was supposed to fix: ~(alignment - 1UL) creates a mask
that is an unsigned long, which is unsigned and therefore not sign
extended when promoted to u64 (if long is 32 bits).
I think the right fix is to use 1L instead of 1UL, to avoid the
possibility of turning the mask unsigned by accident. This will still
fix the original problem, without breaking code that uses ALIGN() the
second way.
This second construct is actually used in the amso1100 driver, so that
driver does not work on 32-bit architectures right now. Unfortunately
almost everyone using it runs 64-bit kernels, so this regression was
not noticed until now.
Signed-off-by: Roland Dreier <[email protected]>
---
If it's too late to merge this for 2.6.19, I can work around this in
the amso1100 driver by doing ALIGN(addr, (u64) alignment) instead.
Let me know if I should send that patch.
However I think this should go in for 2.6.20 at least, since I see
only danger with no benefit from having the constant 1 be unsigned.
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 24b6111..cc542d3 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -31,7 +31,7 @@ #define ULLONG_MAX (~0ULL)
#define STACK_MAGIC 0xdeadbeef
#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
-#define ALIGN(x,a) (((x)+(a)-1UL)&~((a)-1UL))
+#define ALIGN(x,a) (((x)+(a)-1L)&~((a)-1L))
#define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
#define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
From: Roland Dreier <[email protected]>
Date: Fri, 24 Nov 2006 21:40:14 -0800
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 24b6111..cc542d3 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -31,7 +31,7 @@ #define ULLONG_MAX (~0ULL)
> #define STACK_MAGIC 0xdeadbeef
>
> #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> -#define ALIGN(x,a) (((x)+(a)-1UL)&~((a)-1UL))
> +#define ALIGN(x,a) (((x)+(a)-1L)&~((a)-1L))
Perhaps a better way to fix this is to use
typeof() like other similar macros do.
> Perhaps a better way to fix this is to use
> typeof() like other similar macros do.
I tried doing
#define ALIGN(x,a) \
({ \
typeof(x) _a = (a); \
((x) + _a - 1) & ~(_a - 1); \
})
but that won't compile because of <net/neighbour.h>:
unsigned char ha[ALIGN(MAX_ADDR_LEN, sizeof(unsigned long))];
gcc says:
/scratch/Ksrc/linux-merge/include/net/neighbour.h:104: error: braced-group within expression allowed only inside a function
I guess that could be fixed by changing that declaration but now this
is starting to feel like early 2.6.20 material.
- R.
From: Roland Dreier <[email protected]>
Date: Sat, 25 Nov 2006 14:56:22 -0800
> > Perhaps a better way to fix this is to use
> > typeof() like other similar macros do.
>
> I tried doing
>
> #define ALIGN(x,a) \
> ({ \
> typeof(x) _a = (a); \
> ((x) + _a - 1) & ~(_a - 1); \
> })
>
> but that won't compile because of <net/neighbour.h>:
You would need to also cast the constants with typeof() to.
But yes, given the array sizing case in the neighbour code,
perhaps we can use your original patch for now. Feel free
to push that to Linus.
> You would need to also cast the constants with typeof() to.
Why? I'm not much of a C language lawyer, but I would have thought
that in something like
(x) + _a - 1
the "1" will be promoted to the type of the rest of the expression
without any explicit cast. I tested the unsigned long/unsigned int
and u64/int cases of ALIGN(), and my macro with typeof() works for
both of those cases at least.
> But yes, given the array sizing case in the neighbour code,
> perhaps we can use your original patch for now. Feel free
> to push that to Linus.
akpm is CC'ed on this thread. Andrew, are you going to pick this up?
Thanks,
Roland
On Sat, 25 Nov 2006 15:09:38 -0800
Roland Dreier <[email protected]> wrote:
> > But yes, given the array sizing case in the neighbour code,
> > perhaps we can use your original patch for now. Feel free
> > to push that to Linus.
>
> akpm is CC'ed on this thread. Andrew, are you going to pick this up?
Changes this late in the piece rather hurt.
Your proposed change is still wrong for long longs, isn't it?
On Sat, Nov 25, 2006 at 03:05:00PM -0800, David Miller wrote:
> From: Roland Dreier <[email protected]>
> Date: Sat, 25 Nov 2006 14:56:22 -0800
>
> > > Perhaps a better way to fix this is to use
> > > typeof() like other similar macros do.
> >
> > I tried doing
> >
> > #define ALIGN(x,a) \
> > ({ \
> > typeof(x) _a = (a); \
> > ((x) + _a - 1) & ~(_a - 1); \
> > })
> >
> > but that won't compile because of <net/neighbour.h>:
>
> You would need to also cast the constants with typeof() to.
Oh, for fsck sake...
(typeof(x))((x + a - 1) & ~(a - 1ULL))
> (typeof(x))((x + a - 1) & ~(a - 1ULL))
Yes I was being stupid thinking I needed a temporary variable to use
typeof. But what does the cast to typeof(x) accomplish if we write
things the way you suggested above? It seems that the right things is
really just
(((x) + (a) - 1) & ~((typeof(x)) (a) - 1))
- R.
On Sat, Nov 25, 2006 at 05:17:08PM -0800, Roland Dreier wrote:
> > (typeof(x))((x + a - 1) & ~(a - 1ULL))
>
> Yes I was being stupid thinking I needed a temporary variable to use
> typeof. But what does the cast to typeof(x) accomplish if we write
> things the way you suggested above? It seems that the right things is
> really just
>
> (((x) + (a) - 1) & ~((typeof(x)) (a) - 1))
Better have type of result independent of that of a.
> Changes this late in the piece rather hurt.
Fair enough. This was a really close call to me but let's leave it
for 2.6.19. I'll ask Linus to merge the patch below to fix amso1100.
I'm a little worried that other such uses might be lurking in the
tree but I guess no one has complained...
> Your proposed change is still wrong for long longs, isn't it?
Yes, it would fail for aligning long long with an unsigned long
alignment on 32-bits. But that's broken in the current tree too.
I'll post a patch that should work for everything -- how about if you
queue that for 2.6.20-early?
- R.
diff --git a/drivers/infiniband/hw/amso1100/c2_provider.c b/drivers/infiniband/hw/amso1100/c2_provider.c
index fef9727..d54b284 100644
--- a/drivers/infiniband/hw/amso1100/c2_provider.c
+++ b/drivers/infiniband/hw/amso1100/c2_provider.c
@@ -368,7 +368,7 @@ static struct ib_mr *c2_reg_phys_mr(stru
total_len += buffer_list[i].size;
pbl_depth += ALIGN(buffer_list[i].size,
- (1 << page_shift)) >> page_shift;
+ (1ull << page_shift)) >> page_shift;
}
page_list = vmalloc(sizeof(u64) * pbl_depth);
@@ -383,7 +383,7 @@ static struct ib_mr *c2_reg_phys_mr(stru
int naddrs;
naddrs = ALIGN(buffer_list[i].size,
- (1 << page_shift)) >> page_shift;
+ (1ull << page_shift)) >> page_shift;
for (k = 0; k < naddrs; k++)
page_list[j++] = (buffer_list[i].addr +
(k << page_shift));
Commit 4c8bd7ee ("Do not truncate to 'int' in ALIGN() macro.") was
merged to fix the case of code like the following:
unsigned long addr;
unsigned int alignment;
addr = ALIGN(addr, alignment);
The original ALIGN macro calculated a mask as ~(alignment - 1), and
when alignment is just an int, this creates an int mask. If alignment
is also unsigned, then this mask will not be sign extended when
promoted to a long, which leads to the code above chopping off the top
half of addr when long is 64 bits.
However, the changed ALIGN macro, which computes the mask as
~(alignment - 1UL) actually breaks code like the following when long
is 32 bits:
u64 addr;
int alignment;
addr = ALIGN(addr, alignment);
The reason this breaks is pretty much the same as the original bug
that the change was supposed to fix: ~(alignment - 1UL) creates a mask
that is an unsigned long, which is not sign extended when promoted to
u64 (if long is 32 bits).
As suggested by Dave Miller and Al Viro, I fixed this by having the
ALIGN macro make sure the alignment is promoted to the same type as
the value being aligned before doing the negation.
This second construct is actually used in the amso1100 driver, so that
driver does not work on 32-bit architectures right now. Unfortunately
almost everyone using it runs 64-bit kernels, so this regression was
not noticed until now.
Signed-off-by: Roland Dreier <[email protected]>
---
Patch updated as suggested by Dave Miller and Al Viro...
can we merge this for 2.6.20?
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 24b6111..80955b3 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -31,7 +31,7 @@ #define ULLONG_MAX (~0ULL)
#define STACK_MAGIC 0xdeadbeef
#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
-#define ALIGN(x,a) (((x)+(a)-1UL)&~((a)-1UL))
+#define ALIGN(x,a) ((typeof(x)) (((x) + (a) - 1) & ~((typeof(x)) (a) - 1)))
#define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
#define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
On Sun, 26 Nov 2006 11:10:23 -0800
Roland Dreier <[email protected]> wrote:
> Commit 4c8bd7ee ("Do not truncate to 'int' in ALIGN() macro.") was
> merged to fix the case of code like the following:
>
> unsigned long addr;
> unsigned int alignment;
> addr = ALIGN(addr, alignment);
>
> The original ALIGN macro calculated a mask as ~(alignment - 1), and
> when alignment is just an int, this creates an int mask. If alignment
> is also unsigned, then this mask will not be sign extended when
> promoted to a long, which leads to the code above chopping off the top
> half of addr when long is 64 bits.
>
> However, the changed ALIGN macro, which computes the mask as
> ~(alignment - 1UL) actually breaks code like the following when long
> is 32 bits:
>
> u64 addr;
> int alignment;
> addr = ALIGN(addr, alignment);
>
> The reason this breaks is pretty much the same as the original bug
> that the change was supposed to fix: ~(alignment - 1UL) creates a mask
> that is an unsigned long, which is not sign extended when promoted to
> u64 (if long is 32 bits).
>
> As suggested by Dave Miller and Al Viro, I fixed this by having the
> ALIGN macro make sure the alignment is promoted to the same type as
> the value being aligned before doing the negation.
>
> This second construct is actually used in the amso1100 driver, so that
> driver does not work on 32-bit architectures right now. Unfortunately
> almost everyone using it runs 64-bit kernels, so this regression was
> not noticed until now.
>
> Signed-off-by: Roland Dreier <[email protected]>
>
> ---
> Patch updated as suggested by Dave Miller and Al Viro...
>
> can we merge this for 2.6.20?
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 24b6111..80955b3 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -31,7 +31,7 @@ #define ULLONG_MAX (~0ULL)
> #define STACK_MAGIC 0xdeadbeef
>
> #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> -#define ALIGN(x,a) (((x)+(a)-1UL)&~((a)-1UL))
> +#define ALIGN(x,a) ((typeof(x)) (((x) + (a) - 1) & ~((typeof(x)) (a) - 1)))
> #define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
> #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
> #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
I'd be inclined to merge it for 2.6.19. Is everyone OK with it?
> I'd be inclined to merge it for 2.6.19. Is everyone OK with it?
I'm OK with that -- your previous email made me thing you didn't want
to, but I think the risks are rather low, and there's a least a chance
that we'll fix some obscure regression.
On Sun, 26 Nov 2006, Andrew Morton wrote:
>
> I'd be inclined to merge it for 2.6.19. Is everyone OK with it?
I'd _much_ rather make it more readable while at it. Something like this
instead, which has just _one_ "typeof" cast, and at least to me makes it a
lot more obvious what is going on (aka "to align something, use a mask
that is of the same type as the thing to be aligned..").
Maybe it's just me, but I prefer to separate out the actual "action" from
the "type fluff" around it.
Linus
---
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 24b6111..b9b5e4b 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -30,8 +30,10 @@ extern const char linux_banner[];
#define STACK_MAGIC 0xdeadbeef
+#define ALIGN(x,a) __ALIGN_MASK(x,(typeof(x))(a)-1)
+#define __ALIGN_MASK(x,mask) (((x)+(mask))&~(mask))
+
#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
-#define ALIGN(x,a) (((x)+(a)-1UL)&~((a)-1UL))
#define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
#define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
> +#define ALIGN(x,a) __ALIGN_MASK(x,(typeof(x))(a)-1)
> +#define __ALIGN_MASK(x,mask) (((x)+(mask))&~(mask))
Fine by me, but it loses the extra (typeof(x)) cast that Al wanted to
make sure that the result of ALIGN() is not wider than x.
- R.
On Sun, 26 November 2006 12:26:08 -0800, Roland Dreier wrote:
>
> > +#define ALIGN(x,a) __ALIGN_MASK(x,(typeof(x))(a)-1)
> > +#define __ALIGN_MASK(x,mask) (((x)+(mask))&~(mask))
>
> Fine by me, but it loses the extra (typeof(x)) cast that Al wanted to
> make sure that the result of ALIGN() is not wider than x.
Not a big deal, is it?
#define ALIGN(x,a) (typeof(x))__ALIGN_MASK(x,(typeof(x))(a)-1)
#define __ALIGN_MASK(x,mask) (((x)+(mask))&~(mask))
J?rn
--
Don't worry about people stealing your ideas. If your ideas are any good,
you'll have to ram them down people's throats.
-- Howard Aiken quoted by Ken Iverson quoted by Jim Horning quoted by
Raph Levien, 1979
On Sun, 26 Nov 2006, Roland Dreier wrote:
>
> > +#define ALIGN(x,a) __ALIGN_MASK(x,(typeof(x))(a)-1)
> > +#define __ALIGN_MASK(x,mask) (((x)+(mask))&~(mask))
>
> Fine by me, but it loses the extra (typeof(x)) cast that Al wanted to
> make sure that the result of ALIGN() is not wider than x.
Well, since "mask" is now made to be of the same type as "x", every
sub-expression actually has the same type, modulo the normal C behaviour
of "expand to at least "int".
So arguably, the result is _more_ like a normal C operation this way.
Type-wise, the "ALIGN()" macro acts like any other C operation (ie if you
feed it an "unsigned char", the end result is an "int" due to the normal C
type widening that happens for all C operations).
But I don't care horribly much. Al may have some other reasons to _not_
want the normal C type expansion to happen (ie maybe he does something
unnatural with sparse ;)
Linus
On Sun, Nov 26, 2006 at 02:20:10PM -0800, Linus Torvalds wrote:
> So arguably, the result is _more_ like a normal C operation this way.
> Type-wise, the "ALIGN()" macro acts like any other C operation (ie if you
> feed it an "unsigned char", the end result is an "int" due to the normal C
> type widening that happens for all C operations).
>
> But I don't care horribly much. Al may have some other reasons to _not_
> want the normal C type expansion to happen (ie maybe he does something
> unnatural with sparse ;)
Type expansion will happen as soon as you do any arithmetics (or passing
as argument) anyway.
It's actually more of "typeof() has interesting interactions with
other gccisms" kind of thing and general dislike of using that beast
more than absolutely necessary.
Not the #1 on my list of the worst gccisms we are using (that would be
({...}) with its insane semantics and interesting ways to get gcc puke
its guts out), but still pretty high there...
ObFun: what's the type of ({struct {int x,y;} a = {1,2}; a;}) and
how comes that we can say
({struct {int x,y;} a = {1,2}; a;}).y
and get gcc eat it up and evaluate that to 2? Note that we are doing
a very obvious violation of scope rules - WTF _is_ .y in scope where
we have no visible declaration of any structure with field that would
have such name?
IOW, gcc allows type to leak out of scope it's been defined in (and
typeof adds even more fun to the picture). It not only goes against
a _lot_ in C, it's actually not thought through by gcc folks. Just
try to mix that with variable-length arrays and watch it blow up
in interesting ways...