LinuxLists.cc - Large block device patch, part 1 of 9

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

On Tue, Aug 27, 2002 at 03:23:04PM +0000, Pavel Machek wrote:
> Hi!
>
> > Then the following works properly without ugly casts or warnings:
> >
> > __u64 val = 1;
> >
> > printk("at least "PFU64" of your u64s are belong to us\n", val);
>
> Casts are ugly but this looks even worse. I'd go for casts.

Casts override the few type checking abilities the compiler gives us. At
least with the PFU64 style, we'll get warnings when someone changes a variable
into a pointer without remembering to update the printk.

-ben
--
"You will be reincarnated as a toad; and you will be much happier."

2002-08-27 23:36:09

by Peter Chubb

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

>>>>> "Ben" == Benjamin LaHaise <[email protected]> writes:

Ben> On Tue, Aug 27, 2002 at 03:23:04PM +0000, Pavel Machek wrote:
>> Hi!
>>
>> > Then the following works properly without ugly casts or warnings:
>> >
>> > __u64 val = 1;
>> >
>> > printk("at least "PFU64" of your u64s are belong to us\n", val);
>>
>> Casts are ugly but this looks even worse. I'd go for casts.

Ben> Casts override the few type checking abilities the compiler gives
Ben> us. At least with the PFU64 style, we'll get warnings when
Ben> someone changes a variable into a pointer without remembering to
Ben> update the printk.

We did go through all this after the first or second incarnation of
this patch, back in May. After going around in circles for a bit, the
use of casts seemed then to win the day.

If Linus has a major objection I'll redo things.

The main advantage of casts, to me at present, is it allows the
patches to be incremental. Not all places that should be sector_t are
yet --- some are int, and some are long. Casting them all to unsigned
long long and printing with %llu works now, before all the places are
changed (in patch 3 or 4 IIRC).

See the thread that starts at
http://marc.theaimsgroup.com/?l=linux-kernel&m=102100347212072&w=2

--
Dr Peter Chubb [email protected]
You are lost in a maze of BitKeeper repositories, all almost the same.

2002-09-03 10:00:55

by NeilBrown

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

On Tuesday August 27, [email protected] wrote:
> On Tue, Aug 27, 2002 at 03:23:04PM +0000, Pavel Machek wrote:
> > Hi!
> >
> > > Then the following works properly without ugly casts or warnings:
> > >
> > > __u64 val = 1;
> > >
> > > printk("at least "PFU64" of your u64s are belong to us\n", val);
> >
> > Casts are ugly but this looks even worse. I'd go for casts.
>
> Casts override the few type checking abilities the compiler gives us. At
> least with the PFU64 style, we'll get warnings when someone changes a variable
> into a pointer without remembering to update the printk.
>

You could have the best of both worlds with:

static inline long long llsect(sector_t sector) { return (long long)sector;}

and then
printk("The sector number is %Lu.", llsect(sect_num));

Effectively, this is a type-safe cast. You still get the warning, but
it looks more like the C that we are used to.

NeilBrown

2002-09-03 15:51:58

by Linus Torvalds

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

On Tue, 3 Sep 2002, Neil Brown wrote:
>
> Effectively, this is a type-safe cast. You still get the warning, but
> it looks more like the C that we are used to.

I wonder if the right answer isn't to just make things like "__u64" be
"long long" even on 64-bit architectures (at least those on which it is 64
bit, of course. I _think_ that's true of all of them). And then just use
"llu" for it all.

Of course, the really _best_ option would be to have gcc's printf string
format be extensible and dynamic.

Davem, is sparc64 "long long" 64-bit?

Linus

2002-09-03 16:17:18

by Thunder from the hill

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

Hi,

On Tue, 3 Sep 2002, Linus Torvalds wrote:
> Davem, is sparc64 "long long" 64-bit?

Yes...

Thunder
--
--./../...-/. -.--/---/..-/.-./..././.-../..-. .---/..-/.../- .-
--/../-./..-/-/./--..-- ../.----./.-../.-.. --./../...-/. -.--/---/..-
.- -/---/--/---/.-./.-./---/.--/.-.-.-
--./.-/-.../.-./.././.-../.-.-.-

2002-09-03 16:19:13

by Andi Kleen

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

Linus Torvalds <[email protected]> writes:

> On Tue, 3 Sep 2002, Neil Brown wrote:
> >
> > Effectively, this is a type-safe cast. You still get the warning, but
> > it looks more like the C that we are used to.
>
> I wonder if the right answer isn't to just make things like "__u64" be
> "long long" even on 64-bit architectures (at least those on which it is 64
> bit, of course. I _think_ that's true of all of them). And then just use
> "llu" for it all.

x86-64 does that already. I did it originally to fix some printk warnings.
But it caused even more. I didn't bother then to change it back. Doesn't
seem to have too many bad side effects at least.

-Andi

2002-09-03 16:26:08

by Linus Torvalds

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

On 3 Sep 2002, Andi Kleen wrote:
>
> x86-64 does that already. I did it originally to fix some printk warnings.
> But it caused even more. I didn't bother then to change it back. Doesn't
> seem to have too many bad side effects at least.

The printk warnings should be easy to fix once everybody uses the same
types - I think we right now have workarounds exactly for 64-bit machines
where w check BITS_PER_LONG and use different formats for them (exactly
because they historically have _not_ had the same types as the 32-bit
machines).

However, if anybody on the list is hacking gcc, the best option really
would be to just allow better control over gcc printf formats. I have
wanted that in user space too at times. And it doesn't matter if it only
happens in new versions of gcc - we can disable the warning altogether for
old gcc's, as long as enough people have the new gcc to catch new
offenders..

(I'd _love_ to be able to add printk modifiers for other common types in
the kernel, like doing the NIPQUAD thing etc inside printk() instead of
having it pollute the callers. All of which has been avoided because of
the hardcoded gcc format warning..)

Linus

2002-09-03 16:38:36

by Nikita Danilov

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

Linus Torvalds writes:
>
>
> On 3 Sep 2002, Andi Kleen wrote:
> >
> > x86-64 does that already. I did it originally to fix some printk warnings.
> > But it caused even more. I didn't bother then to change it back. Doesn't
> > seem to have too many bad side effects at least.
>
> The printk warnings should be easy to fix once everybody uses the same
> types - I think we right now have workarounds exactly for 64-bit machines
> where w check BITS_PER_LONG and use different formats for them (exactly
> because they historically have _not_ had the same types as the 32-bit
> machines).
>
> However, if anybody on the list is hacking gcc, the best option really
> would be to just allow better control over gcc printf formats. I have
> wanted that in user space too at times. And it doesn't matter if it only

See <printf.h>: register_printf_function(). -Wformat doesn't know about
new specifiers, though.

> happens in new versions of gcc - we can disable the warning altogether for
> old gcc's, as long as enough people have the new gcc to catch new
> offenders..
>
> (I'd _love_ to be able to add printk modifiers for other common types in
> the kernel, like doing the NIPQUAD thing etc inside printk() instead of
> having it pollute the callers. All of which has been avoided because of
> the hardcoded gcc format warning..)
>
> Linus

Nikita.

>

2002-09-03 16:51:55

by Linus Torvalds

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

On Tue, 3 Sep 2002, Nikita Danilov wrote:
>
> See <printf.h>: register_printf_function(). -Wformat doesn't know about
> new specifiers, though.

That's just an internal way to add ways to print out different numbers to
the libc version of printf - it doesn't actually tell _gcc_ what the
modifiers are.

Linus

2002-09-03 19:36:18

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

On Tuesday 03 September 2002 12:01, Neil Brown wrote:
> On Tuesday August 27, [email protected] wrote:
> > On Tue, Aug 27, 2002 at 03:23:04PM +0000, Pavel Machek wrote:
> > > Hi!
> > >
> > > > Then the following works properly without ugly casts or warnings:
> > > >
> > > > __u64 val = 1;
> > > >
> > > > printk("at least "PFU64" of your u64s are belong to us\n", val);
> > >
> > > Casts are ugly but this looks even worse. I'd go for casts.
> >
> > Casts override the few type checking abilities the compiler gives us. At
> > least with the PFU64 style, we'll get warnings when someone changes a variable
> > into a pointer without remembering to update the printk.
> >
>
> You could have the best of both worlds with:
>
> static inline long long llsect(sector_t sector) { return (long long)sector;}
>
> and then
> printk("The sector number is %Lu.", llsect(sect_num));
>
> Effectively, this is a type-safe cast. You still get the warning, but
> it looks more like the C that we are used to.

We've been through this before. Last time, the winning solution was:

printk("at least %lli of your u64s are belong to us\n", (long long) sect_num);

and I expect it will be this time too. It's just a printk! Who cares if it
wastes a few bytes. It's even conceivable that if we use this idiom heavily
enough, some gcc boffin will take the time to optimize away the useless
conversions.

--
Daniel

2002-09-03 19:46:13

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

On Tue, Sep 03, 2002 at 09:42:56PM +0200, Daniel Phillips wrote:
> and I expect it will be this time too. It's just a printk! Who cares if it
> wastes a few bytes. It's even conceivable that if we use this idiom heavily
> enough, some gcc boffin will take the time to optimize away the useless
> conversions.

The issue of casting was never successfully treated by any of the approaches
that were presented, except by Neil's llsect() function. It's shorter than
(long long)sect and has type safety, so let's just use it.

-ben
--
"You will be reincarnated as a toad; and you will be much happier."

2002-09-03 19:55:51

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

On Tuesday 03 September 2002 21:50, Benjamin LaHaise wrote:
> On Tue, Sep 03, 2002 at 09:42:56PM +0200, Daniel Phillips wrote:
> > and I expect it will be this time too. It's just a printk! Who cares if it
> > wastes a few bytes. It's even conceivable that if we use this idiom heavily
> > enough, some gcc boffin will take the time to optimize away the useless
> > conversions.
>
> The issue of casting was never successfully treated by any of the approaches
> that were presented, except by Neil's llsect() function. It's shorter than
> (long long)sect and has type safety, so let's just use it.

If you must have a clever macro:

#define lli(foo) (long long int) (foo)
#define llu(foo) (long long unsigned) (foo)

The %lli/%llu have to be there in the format string (modulo as-yet-uninvented
printk hackery) so the cast might as well be there as well.

--
Daniel

2002-09-03 19:59:42

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

On Tue, Sep 03, 2002 at 10:02:38PM +0200, Daniel Phillips wrote:
> If you must have a clever macro:
>
> #define lli(foo) (long long int) (foo)
> #define llu(foo) (long long unsigned) (foo)
>
> The %lli/%llu have to be there in the format string (modulo as-yet-uninvented
> printk hackery) so the cast might as well be there as well.

Do you not read the messages in a thread before posting?

static inline long long llsect(sector_t sect) { return sect; }

was the suggestion.

-ben
--
"You will be reincarnated as a toad; and you will be much happier."

2002-09-03 20:00:46

by Thunder from the hill

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

Hi,

On Tue, 3 Sep 2002, Daniel Phillips wrote:
> If you must have a clever macro:
>
> #define lli(foo) (long long int) (foo)
> #define llu(foo) (long long unsigned) (foo)

Type safety not given.

Thunder
--
--./../...-/. -.--/---/..-/.-./..././.-../..-. .---/..-/.../- .-
--/../-./..-/-/./--..-- ../.----./.-../.-.. --./../...-/. -.--/---/..-
.- -/---/--/---/.-./.-./---/.--/.-.-.-
--./.-/-.../.-./.././.-../.-.-.-

2002-09-03 20:12:43

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

On Tuesday 03 September 2002 22:04, Benjamin LaHaise wrote:
> On Tue, Sep 03, 2002 at 10:02:38PM +0200, Daniel Phillips wrote:
> > If you must have a clever macro:
> >
> > #define lli(foo) (long long int) (foo)
> > #define llu(foo) (long long unsigned) (foo)
> >
> > The %lli/%llu have to be there in the format string (modulo as-yet-uninvented
> > printk hackery) so the cast might as well be there as well.
>
> Do you not read the messages in a thread before posting?
>
> static inline long long llsect(sector_t sect) { return sect; }
>
> was the suggestion.

I didn't like it, I thought it was awkward. This is better:

printk("%lli ", llu(sect));

and

printk("%lli ", (long long unsigned) sect);

is probably better yet. Why get fancy.

--
Daniel

2002-09-03 20:13:19

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

On Tuesday 03 September 2002 22:05, Thunder from the hill wrote:
> Hi,
>
> On Tue, 3 Sep 2002, Daniel Phillips wrote:
> > If you must have a clever macro:
> >
> > #define lli(foo) (long long int) (foo)
> > #define llu(foo) (long long unsigned) (foo)
>
> Type safety not given.

It's a printk for crying out loud.

--
Daniel

2002-09-03 20:18:11

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

On Tuesday 03 September 2002 22:19, Daniel Phillips wrote:
> On Tuesday 03 September 2002 22:04, Benjamin LaHaise wrote:
> > On Tue, Sep 03, 2002 at 10:02:38PM +0200, Daniel Phillips wrote:
> > > If you must have a clever macro:
> > >
> > > #define lli(foo) (long long int) (foo)
> > > #define llu(foo) (long long unsigned) (foo)
> > >
> > > The %lli/%llu have to be there in the format string (modulo as-yet-uninvented
> > > printk hackery) so the cast might as well be there as well.
> >
> > Do you not read the messages in a thread before posting?
> >
> > static inline long long llsect(sector_t sect) { return sect; }
> >
> > was the suggestion.
>
> I didn't like it, I thought it was awkward. This is better:
>
> printk("%lli ", llu(sect));

Err, %llu, and big deal.

--
Daniel

2002-09-03 23:02:31

by NeilBrown

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

On Tuesday September 3, [email protected] wrote:
>
> On Tue, 3 Sep 2002, Neil Brown wrote:
> >
> > Effectively, this is a type-safe cast. You still get the warning, but
> > it looks more like the C that we are used to.
>
> I wonder if the right answer isn't to just make things like "__u64" be
> "long long" even on 64-bit architectures (at least those on which it is 64
> bit, of course. I _think_ that's true of all of them). And then just use
> "llu" for it all.

The thing is that the patch in question wants to print a "sector_t",
not a "__u64".
sector_t can be u32 (on 32 bit machines that don't need big devices
and don't want the performance hit) or can be u64 (elsewhere).

And isn't saying "long long is 64bits" just as bad as all the
pre-alpha code that thought "long" was 32 bits, or the PDP code that
knew that "int" was 16 bits?

NeilBrown

2002-09-04 23:59:50

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

On Thu, Sep 05, 2002 at 09:04:01AM +1000, Peter Chubb wrote:
> Unfortunately, this doesn't really buy you much ---- standard C type
> promotion rules mean that whatever (within reason) you pass to
> llsect() will work without warning.

Not if someone changes a variable to a pointer by accident without
updating the printk. It does happen.

-ben
--
"You will be reincarnated as a toad; and you will be much happier."

2002-09-05 00:32:17

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

On Thursday 05 September 2002 02:04, Benjamin LaHaise wrote:
> On Thu, Sep 05, 2002 at 09:04:01AM +1000, Peter Chubb wrote:
> > Unfortunately, this doesn't really buy you much ---- standard C type
> > promotion rules mean that whatever (within reason) you pass to
> > llsect() will work without warning.
>
> Not if someone changes a variable to a pointer by accident without
> updating the printk. It does happen.

The thing is, I don't see why we should be building castles and cathedrals
around printk. Just cast to the wider value, if you get it wrong you have
lost exactly what? Are people feeding the output of dmesg into scripts
that their systems depend upon? If so, we need to let evolution do its
work.

If this had something to do with how the kernel worked, it might be worth
inventing a new feature.

--
Daniel

2002-09-05 03:58:11

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

On Thursday 05 September 2002 03:31, Benjamin LaHaise wrote:
> On Thu, Sep 05, 2002 at 02:38:58AM +0200, Daniel Phillips wrote:
> > The thing is, I don't see why we should be building castles and cathedrals
> > around printk. Just cast to the wider value, if you get it wrong you have
> > lost exactly what? Are people feeding the output of dmesg into scripts
> > that their systems depend upon? If so, we need to let evolution do its
> > work.
>
> Why do it the broken way when you can do it a non-broken way? Arguing in
> favour of having it broken by design isn't something I really understand.

Because you're only fixing the printk, and with an inadequate solution at
that. Could we please fix something that matters?

--
Daniel

2002-09-05 01:27:23

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

On Thu, Sep 05, 2002 at 02:38:58AM +0200, Daniel Phillips wrote:
> The thing is, I don't see why we should be building castles and cathedrals
> around printk. Just cast to the wider value, if you get it wrong you have
> lost exactly what? Are people feeding the output of dmesg into scripts
> that their systems depend upon? If so, we need to let evolution do its
> work.

Why do it the broken way when you can do it a non-broken way? Arguing in
favour of having it broken by design isn't something I really understand.
Anyways, I'll refrain from posting any further comments on this thread.

-ben
--
"You will be reincarnated as a toad; and you will be much happier."

2002-12-02 23:56:33

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

Followup to: <[email protected]>
By author: Linus Torvalds <[email protected]>
In newsgroup: linux.dev.kernel
>
> I wonder if the right answer isn't to just make things like "__u64" be
> "long long" even on 64-bit architectures (at least those on which it is 64
> bit, of course. I _think_ that's true of all of them). And then just use
> "llu" for it all.
>
> Of course, the really _best_ option would be to have gcc's printf string
> format be extensible and dynamic.
>
> Davem, is sparc64 "long long" 64-bit?
>

Some C libraries have "Ixx" as an extension meaning (u)intXX_t -- it's
used in the same way as the other size modifiers, i.e. %I64x instead
of %lx. This might be a useful extension to mimic.

-hpa
--
<[email protected]> at work, <[email protected]> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt <[email protected]>

2002-12-02 23:58:07

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

Followup to: <E17mJZh-0005jw-00@starship>
By author: Daniel Phillips <[email protected]>
In newsgroup: linux.dev.kernel
>
> We've been through this before. Last time, the winning solution was:
>
> printk("at least %lli of your u64s are belong to us\n", (long long) sect_num);
>
> and I expect it will be this time too. It's just a printk! Who cares if it
> wastes a few bytes. It's even conceivable that if we use this idiom heavily
> enough, some gcc boffin will take the time to optimize away the useless
> conversions.
>

Why can't we use the C99 standard:

printk("at least %ji of your u64s are belong to us\n", (uintmax_t) sect_num);

I, for one, would be rather happy at having <inttypes.h> available in
the kernel, as either an alternative or instead of the [su]XX/__[su]XX types.

-hpa
--
<[email protected]> at work, <[email protected]> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt <[email protected]>

2002-12-02 23:59:41

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

Followup to: <[email protected]>
By author: Linus Torvalds <[email protected]>
In newsgroup: linux.dev.kernel
>
> The printk warnings should be easy to fix once everybody uses the same
> types - I think we right now have workarounds exactly for 64-bit machines
> where w check BITS_PER_LONG and use different formats for them (exactly
> because they historically have _not_ had the same types as the 32-bit
> machines).
>
> However, if anybody on the list is hacking gcc, the best option really
> would be to just allow better control over gcc printf formats. I have
> wanted that in user space too at times. And it doesn't matter if it only
> happens in new versions of gcc - we can disable the warning altogether for
> old gcc's, as long as enough people have the new gcc to catch new
> offenders..
>
> (I'd _love_ to be able to add printk modifiers for other common types in
> the kernel, like doing the NIPQUAD thing etc inside printk() instead of
> having it pollute the callers. All of which has been avoided because of
> the hardcoded gcc format warning..)
>

While we're talking about printk()... is there any reason *not* to
rename it printf()?

-hpa
--
<[email protected]> at work, <[email protected]> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt <[email protected]>

2002-12-03 08:38:24

by Giacomo A. Catenazzi

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

H. Peter Anvin wrote:
>
> While we're talking about printk()... is there any reason *not* to
> rename it printf()?

kprintf would be better, maybe addind the first argument for verbosity level,
instead of being "inlined" in the string. But it is a 2.7 change!

ciao
giacomo

2002-12-03 17:34:12

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

Giacomo Catenazzi wrote:
>
> kprintf would be better
>

Why? We don't have, say, kstrcpy() or ksscanf() for other functions
that are library-equivalent.

-hpa

2002-12-03 17:40:37

by Giacomo A. Catenazzi

[permalink] [raw]

Subject: Re: Large block device patch, part 1 of 9

H. Peter Anvin wrote:
> Giacomo Catenazzi wrote:
>
>>
>> kprintf would be better
>
> >
>
> Why? We don't have, say, kstrcpy() or ksscanf() for other functions
> that are library-equivalent.

k to remember people that it is not the std function, thus not a write to
the standard output. I see more like fprintf (file) and [v]sprintf (string),
we write to a special buffer ("klog").
But if we don't move the verbosity level out of string, also the simple
'printf' would be fine.

ciao
giacomo

2002-12-05 21:09:59