On Mon, 14 Jan 2013 14:32:02 -0800 Kent Overstreet <[email protected]> wrote:
> Bcache: a block layer SSD cache
sparc64 gcc-3.4.5:
drivers/md/bcache/btree.c: In function `bch_btree_read':
drivers/md/bcache/btree.c:266: error: invalid operands to binary +
drivers/md/bcache/btree.c: In function `__btree_write':
drivers/md/bcache/btree.c:379: error: invalid operands to binary +
drivers/md/bcache/btree.c: In function `btree_node_free':
drivers/md/bcache/btree.c:980: error: invalid operands to binary +
drivers/md/bcache/btree.c: In function `btree_insert_key':
drivers/md/bcache/btree.c:1857: error: invalid operands to binary +
drivers/md/bcache/btree.c:1857: error: invalid operands to binary +
drivers/md/bcache/btree.c:1859: error: invalid operands to binary +
drivers/md/bcache/btree.c:1859: error: invalid operands to binary +
drivers/md/bcache/btree.c:1864: error: invalid operands to binary +
drivers/md/bcache/btree.c:1864: error: invalid operands to binary +
drivers/md/bcache/btree.c: In function `btree_split':
drivers/md/bcache/btree.c:1934: error: invalid operands to binary +
drivers/md/bcache/btree.c: In function `bch_btree_set_root':
drivers/md/bcache/btree.c:2159: error: invalid operands to binary +
drivers/md/bcache/btree.c: In function `bch_btree_search_recurse':
drivers/md/bcache/btree.c:2262: error: invalid operands to binary +
drivers/md/bcache/btree.c: In function `bch_btree_refill_keybuf':
drivers/md/bcache/btree.c:2330: error: invalid operands to binary +
due to
#define pbtree(b) (&bch_pbtree(b).s[0])
I don't know why this is happening (presumably a gcc glitch), but
returning an 80-byte struct by value from bch_pkey() and bch_pbtree()
is just gruesome. The compiler has to allocate the space on the caller
stack, pass a hidden pointer into the callee and the callee copies its
return value into that caller stack slot. It's slow and consumes stack.
Something different, please.
On Thu, Apr 25, 2013 at 04:17:04PM -0700, Andrew Morton wrote:
> On Mon, 14 Jan 2013 14:32:02 -0800 Kent Overstreet <[email protected]> wrote:
>
> > Bcache: a block layer SSD cache
>
> sparc64 gcc-3.4.5:
>
> drivers/md/bcache/btree.c: In function `bch_btree_read':
> drivers/md/bcache/btree.c:266: error: invalid operands to binary +
> drivers/md/bcache/btree.c: In function `__btree_write':
> drivers/md/bcache/btree.c:379: error: invalid operands to binary +
> drivers/md/bcache/btree.c: In function `btree_node_free':
> drivers/md/bcache/btree.c:980: error: invalid operands to binary +
> drivers/md/bcache/btree.c: In function `btree_insert_key':
> drivers/md/bcache/btree.c:1857: error: invalid operands to binary +
> drivers/md/bcache/btree.c:1857: error: invalid operands to binary +
> drivers/md/bcache/btree.c:1859: error: invalid operands to binary +
> drivers/md/bcache/btree.c:1859: error: invalid operands to binary +
> drivers/md/bcache/btree.c:1864: error: invalid operands to binary +
> drivers/md/bcache/btree.c:1864: error: invalid operands to binary +
> drivers/md/bcache/btree.c: In function `btree_split':
> drivers/md/bcache/btree.c:1934: error: invalid operands to binary +
> drivers/md/bcache/btree.c: In function `bch_btree_set_root':
> drivers/md/bcache/btree.c:2159: error: invalid operands to binary +
> drivers/md/bcache/btree.c: In function `bch_btree_search_recurse':
> drivers/md/bcache/btree.c:2262: error: invalid operands to binary +
> drivers/md/bcache/btree.c: In function `bch_btree_refill_keybuf':
> drivers/md/bcache/btree.c:2330: error: invalid operands to binary +
>
> due to
>
> #define pbtree(b) (&bch_pbtree(b).s[0])
>
> I don't know why this is happening (presumably a gcc glitch), but
> returning an 80-byte struct by value from bch_pkey() and bch_pbtree()
> is just gruesome. The compiler has to allocate the space on the caller
> stack, pass a hidden pointer into the callee and the callee copies its
> return value into that caller stack slot. It's slow and consumes stack.
>
> Something different, please.
Well, it is kind of... perverse but really the compiler's doing exactly
what I would've had to do otherwise - stick a char buf[80] on the
caller's stack and pass it to bch_pbtree(). With the caveat that I
haven't looked at the generated code.
As far as I can tell the only real improvement would be to add a %p
format string to vsnprintf, but adding a global extension would obviously be
inappropriate for this. It'd be really nice to have a mechanism for
adding file/module private format strings to vsnprintf, but I haven't
cared enough yet to implement it myself.
Of course if you know a better solution I'm all ears.
Uhm, as for the actual bug - that is a fairly ancient gcc, I wasn't
aware we were supporting compilers that old but I'm sure you wouldn't be
bugging me about it if we weren't...
If you _really_ want me to rip out the macro/struct return hack I
will... but this is just debug code and I hate making it more verbose if
I don't have to.
Otherwise, I'll set up gcc-3.4.5 (hopefully it doesn't have to be a
sparc compiler :P) and see if I can get gcc to stop complaining.
On Fri, 26 Apr 2013 12:46:42 -0700 Kent Overstreet <[email protected]> wrote:
> On Thu, Apr 25, 2013 at 04:17:04PM -0700, Andrew Morton wrote:
> > On Mon, 14 Jan 2013 14:32:02 -0800 Kent Overstreet <[email protected]> wrote:
> >
> ...
> > drivers/md/bcache/btree.c: In function `bch_btree_refill_keybuf':
> > drivers/md/bcache/btree.c:2330: error: invalid operands to binary +
> >
> > due to
> >
> > #define pbtree(b) (&bch_pbtree(b).s[0])
> >
> > I don't know why this is happening (presumably a gcc glitch), but
> > returning an 80-byte struct by value from bch_pkey() and bch_pbtree()
> > is just gruesome. The compiler has to allocate the space on the caller
> > stack, pass a hidden pointer into the callee and the callee copies its
> > return value into that caller stack slot. It's slow and consumes stack.
> >
> > Something different, please.
>
> Well, it is kind of... perverse but really the compiler's doing exactly
> what I would've had to do otherwise - stick a char buf[80] on the
> caller's stack and pass it to bch_pbtree(). With the caveat that I
> haven't looked at the generated code.
That's the more idiomatic way of doing things and yes, the code
generation will be similarly awful.
> As far as I can tell the only real improvement would be to add a %p
> format string to vsnprintf, but adding a global extension would obviously be
> inappropriate for this. It'd be really nice to have a mechanism for
> adding file/module private format strings to vsnprintf, but I haven't
> cared enough yet to implement it myself.
>
> Of course if you know a better solution I'm all ears.
>
> Uhm, as for the actual bug - that is a fairly ancient gcc, I wasn't
> aware we were supporting compilers that old but I'm sure you wouldn't be
> bugging me about it if we weren't...
Documentation/Changes is the official status. It says gcc-3.2+. We're
very slow in updating those version numbers because it's hard.
gcc-3.4.5 for mips fails in the same way.
Why are those things macros anyway? urgh, it's because we want to jam
a string into the caller's stack frame without declaring any of it.
Really I do think it would be better to do away with the C party tricks
and have callers do
char btree_buf[BTREE_BUF_SIZE];
btree_to_text(btree_buf, b);
pr_debug("%s\n", btree_buf);
Nice, simple, explicit, direct and stupid. It might generate
unused-var warnings if DEBUG is undefined but from my reading of
pr_debug() things will be OK.
Then we can poke around at btree_to_text() until gcc-3.4.5 is happy
with it.
On Fri, Apr 26, 2013 at 01:24:38PM -0700, Andrew Morton wrote:
> > As far as I can tell the only real improvement would be to add a %p
> > format string to vsnprintf, but adding a global extension would obviously be
> > inappropriate for this. It'd be really nice to have a mechanism for
> > adding file/module private format strings to vsnprintf, but I haven't
> > cared enough yet to implement it myself.
> >
> > Of course if you know a better solution I'm all ears.
> >
> > Uhm, as for the actual bug - that is a fairly ancient gcc, I wasn't
> > aware we were supporting compilers that old but I'm sure you wouldn't be
> > bugging me about it if we weren't...
>
> Why are those things macros anyway? urgh, it's because we want to jam
> a string into the caller's stack frame without declaring any of it.
Well, it also _used_ to be the case that if DEBUG and
CONFIG_DYNAMIC_DEBUG weren't defined then pr_debug() was defined as an
empty macro, so btree_to_text() wouldn't be called.
But, I just checked and it's been converted to an empty inline
function, and some of this debug code is called from fast paths where
the overhead really does matter :/ argh
> Really I do think it would be better to do away with the C party tricks
> and have callers do
>
> char btree_buf[BTREE_BUF_SIZE];
>
> btree_to_text(btree_buf, b);
> pr_debug("%s\n", btree_buf);
>
> Nice, simple, explicit, direct and stupid. It might generate
> unused-var warnings if DEBUG is undefined but from my reading of
> pr_debug() things will be OK.
>
> Then we can poke around at btree_to_text() until gcc-3.4.5 is happy
> with it.
Well, since pr_debug() isn't an empty macro anymore there goes my main
reason for keeping these macros around.
seems like pr_debug() could still have the no_printk() call inside an
if (0) though...
argh. If there was some way to tell gcc "this function only modifies
this argument so you don't have to call it if the output is never used"
then I'd just convert it to the explicit bufs now. But I really can't
have btree_to_text()/bkey_to_text() getting called in non debug
kernels...
I'm going to mull it over a bit for now, and try and decide what's least
ugly.