On Thu, Feb 17, 2022 at 10:50 AM Jakob Koschel <[email protected]> wrote:
>
> It is unsafe to assume that &req->req != _req can only evaluate
> to false if the break within the list iterator is hit.
I don't understand what problem you are trying to fix.
Since "req" absolutely *has* to be stable for any of this code to be
valid, then "&req->req" is stable and unambiguous too. The *only* way
_req can point to it would be if we finished the iteration properly.
So I don't see the unsafeness.
Note that all this work with "speculative" execution fundamentally CAN
NOT affect semantics of the code, yet this patch makes statements
about exactly that.
That's not how CPU speculation works.
CPU speculation can expose hidden information that is not
"semantically important" (typically through cache access patterns, but
that's not the only way). So it might be exposing information it
shouldn't.
But if speculation is actually changing semantics, then it's no longer
"speculation" - it's just a bug, plain and simple (either a software
bug due to insufficient serialization, or an actual hardware bug).
IOW, I don't want to see these kinds of apparently pointless changes
to list walking. The patches should explain what that SECONDARY hidden
value you try to protect actually is for each case.
This patch is basically not sensible. It just moves code around in a
way that the compiler could have done anyway (or the compiler could
decide to undo). It doesn't explain what the magic protected value is
that shouldn't be leaked, and it leaves the code just looking odd and
pointless.
Linus
> On 17. Feb 2022, at 20:28, Linus Torvalds <[email protected]> wrote:
>
> On Thu, Feb 17, 2022 at 10:50 AM Jakob Koschel <[email protected]> wrote:
>>
>> It is unsafe to assume that &req->req != _req can only evaluate
>> to false if the break within the list iterator is hit.
>
> I don't understand what problem you are trying to fix.
>
> Since "req" absolutely *has* to be stable for any of this code to be
> valid, then "&req->req" is stable and unambiguous too. The *only* way
> _req can point to it would be if we finished the iteration properly.
>
> So I don't see the unsafeness.
>
> Note that all this work with "speculative" execution fundamentally CAN
> NOT affect semantics of the code, yet this patch makes statements
> about exactly that.
I'm sorry for having created the confusion. I made this patch to support
the speculative safe list_for_each_entry() version but it is not actually
related to that. I do believe that this an actual bug and *could*
*potentially* be misused. I'll follow up with an example to illustrate that.
I agree that this has nothing to do with the speculative execution iterator
(apart from making it work) and should best be discussed separately.
I'll attach an example on how I think this code *can* become a problem.
Note that this highly depends on the used compiler and how the struct
offsets are laid out.
>
> That's not how CPU speculation works.
>
> CPU speculation can expose hidden information that is not
> "semantically important" (typically through cache access patterns, but
> that's not the only way). So it might be exposing information it
> shouldn't.
>
> But if speculation is actually changing semantics, then it's no longer
> "speculation" - it's just a bug, plain and simple (either a software
> bug due to insufficient serialization, or an actual hardware bug).
>
> IOW, I don't want to see these kinds of apparently pointless changes
> to list walking. The patches should explain what that SECONDARY hidden
> value you try to protect actually is for each case.
>
> This patch is basically not sensible. It just moves code around in a
> way that the compiler could have done anyway (or the compiler could
> decide to undo). It doesn't explain what the magic protected value is
> that shouldn't be leaked, and it leaves the code just looking odd and
> pointless.
>
> Linus
[ Arnd was already on the participants, but I moved him from 'Cc:' to
'To:', just because I think this is once again tangentially related to
the whole "c99 base" thing ]
On Wed, Feb 23, 2022 at 6:13 AM Jakob <[email protected]> wrote:
>
> I'm sorry for having created the confusion. I made this patch to support
> the speculative safe list_for_each_entry() version but it is not actually
> related to that. I do believe that this an actual bug and *could*
> *potentially* be misused. I'll follow up with an example to illustrate that.
Ok, so this is just a regular bug, plain and simple.
The problem being that the list_for_each_entry() will iterate over
each list entry - but at the end of the loop it will not point at any
entry at all (it will have a pointer value that is related to the
*HEAD* of the list, but that is not necessarily the same kind of entry
that the list members are.
Honestly, I think this kind of fix should have been done entirely separately.
In fact, I think the change to list_for_each_entry() should have been
done not as "fix type speculation", but as a much more interesting
"fix the list iterators".
The whole reason this kind of non-speculative bug can happen is that
we historically didn't have C99-style "declare variables in loops". So
list_for_each_entry() - and all the other ones - fundamentally always
leaks the last HEAD entry out of the loop, simply because we couldn't
declare the iterator variable in the loop itself.
(And by "couldn't", I mean "without making for special syntax": we do
exactly that in "for_each_thread ()" and friends, but they have an
"end_for_each_thread()" thing at the end).
So what I'd personally *really* like to see would be for us to - once
again - look at using "-std=gnu99", and fix the whole "leak final
invalid pointer outside the loop".
Then the type speculation thing would be an entirely separate patch.
Because honestly, I kind of hate the completely random type
speculation patch. It fixes one particular type of loop, and not even
one that seems all that special.
But we still don't do "gnu99", because we had some odd problem with
some ancient gcc versions that broke documented initializers.
I honestly _thought_ we had gotten over that already. I think the
problem cases were gcc-4.9 and older, and now we require gcc-5.1, and
we could just use "--std=gnu99" for the kernel, and we could finally
start using variable declarations in for-statements.
Arnd - remind me, please.. Was there some other problem than just gcc-4.9?
Linus
Note that I changed the location of the struct member 'req' in gr_request
to make this work. Instead of reshuffling struct members this can also be
introduced by simply adding new struct members in certain spots.
In other code locations with the same pattern I didn't have to do that.
Assuming '_req' passed to gr_dequeue() is located just past 'ep' on the
heap, the check could pass even though the list searched is completely
empty.
&req->req for the head element will be an out-of-bounds pointer
that by coincidence or heap massaging is where '_req' is located.
Even if the list is empty the list_for_each_entry() macro will do:
pos = list_first_entry(head, typeof(*pos), member);
resolving all the macros (list_first_entry() etc) it will look like this:
pos = container_of(head->next, typeof(*pos), member)
Since the list is empty head->next == head and container_of() is called on something
that is *not* actually of type gr_request.
Next, the check if the end of the loop is hit is evaluated:
!list_entry_is_head(pos, head, member);
which will stop the loop directly before executing a single iteration.
then using '&req->req' is some bogus pointer pointing just past the struct gr_ep,
where in this case '_req' is located.
The point I'm trying to make: it's probably not safe to rely on the compiler and
that everyone is aware of this risk when adding/removing/reordering struct members.
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/gr_udc.c | 25 +++++++++++++++++++++++++
drivers/usb/gadget/udc/gr_udc.h | 2 +-
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/gr_udc.c b/drivers/usb/gadget/udc/gr_udc.c
index 4b35739d3695..29c662f28428 100644
--- a/drivers/usb/gadget/udc/gr_udc.c
+++ b/drivers/usb/gadget/udc/gr_udc.c
@@ -1718,6 +1718,7 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
ret = -EINVAL;
goto out;
}
+ printk(KERN_INFO "JKL: This does print, but shouldn't");
if (list_first_entry(&ep->queue, struct gr_request, queue) == req) {
/* This request is currently being processed */
@@ -1739,6 +1740,30 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
return ret;
}
+static int __init init_test_jkl3(void)
+{
+ struct gr_ep *ep;
+ struct gr_udc *dev;
+ struct usb_request *_req;
+ void *buffer;
+
+ /* assume the usb_request struct is located just after the 'ep' on the heap */
+ buffer = kzalloc(sizeof(struct gr_ep)+sizeof(struct usb_request), GFP_KERNEL);
+ ep = buffer;
+ _req = buffer+sizeof(struct gr_ep);
+
+ /* setup to call gr_dequeue() */
+ dev = kzalloc(sizeof(struct gr_udc), GFP_KERNEL);
+ ep->dev = dev;
+ ep->dev->driver = 1;
+ INIT_LIST_HEAD(&ep->queue); /* list is empty */
+
+ gr_dequeue(&ep->ep, _req);
+
+ return 0;
+}
+__initcall(init_test_jkl3);
+
/* Helper for gr_set_halt and gr_set_wedge */
static int gr_set_halt_wedge(struct usb_ep *_ep, int halt, int wedge)
{
diff --git a/drivers/usb/gadget/udc/gr_udc.h b/drivers/usb/gadget/udc/gr_udc.h
index 70134239179e..14a18d5b5cf8 100644
--- a/drivers/usb/gadget/udc/gr_udc.h
+++ b/drivers/usb/gadget/udc/gr_udc.h
@@ -159,7 +159,6 @@ struct gr_ep {
};
struct gr_request {
- struct usb_request req;
struct list_head queue;
/* Chain of dma descriptors */
@@ -171,6 +170,7 @@ struct gr_request {
u16 oddlen; /* Size of odd length tail if buffer length is "odd" */
u8 setup; /* Setup packet */
+ struct usb_request req;
};
enum gr_ep0state {
--
2.25.1
On Wed, Feb 23, 2022 at 10:47 AM Linus Torvalds
<[email protected]> wrote:
>
> Arnd - remind me, please.. Was there some other problem than just gcc-4.9?
Hmm. Interesting. I decided to just try it and see for the compiler I
have, and changing the gnu89 to gnu99 I get new warnings
(-Werror=shift-negative-value).
Very annoying. Especially since negative values are in many contexts
actually *safer* than positive ones when used as a mask, because as
long as the top bit is set in 'int', if the end result is then
expanded to some wider type, the top bit stays set.
Example:
unsigned long mask(unsigned long x)
{ return x & (~0 << 5); }
unsigned long badmask(unsigned long x)
{ return x & (~0u << 5); }
One does it properly, the other is buggy.
Now, with an explicit "unsigned long" like this, some clueless
compiler person might say "just use "~0ul", but that's completely
wrong - because quite often the type is *not* this visible, and the
signed version works *regardless* of type.
So this Werror=shift-negative-value warning seems to be actively
detrimental, and I'm not seeing the reason for it. Can somebody
explain the thinking for that stupid warning?
That said, we seem to only have two cases of it in the kernel, at
least by a x86-64 allmodconfig build. So we could examine the types
there, or we could just add '-Wno-shift-negative-value".
Linus
On Wed, Feb 23, 2022 at 11:23 AM Linus Torvalds
<[email protected]> wrote:
>
> That said, we seem to only have two cases of it in the kernel, at
> least by a x86-64 allmodconfig build.
No, there's more of them, it's just that the build broke early enough
that I didn't see it.
Doing
git grep '\(\(~0\)\|\(-1\)\) <<'
finds a number of them. Some of them have casts in front, so they
wouldn't necessarily trigger this issue, but it's not an entirely
uncommon pattern.
And as mentioned, I think it's a *good* pattern, in that it takes
advantage of the sign-extension of the top bit in any widening use,
when the type might not be obvious (in macros, or when accessing
members of unions or structures, or when using typedefs that hide the
actual type).
So I still think that warning is actively detrimental, and I'm
wondering why it was added (and why 'gnu99' enables it, but 'gnu89'
does not). There's presumably _some_ reason.
Linus
On Wed, Feb 23, 2022 at 03:16:09PM +0100, Jakob wrote:
> Note that I changed the location of the struct member 'req' in gr_request
> to make this work. Instead of reshuffling struct members this can also be
> introduced by simply adding new struct members in certain spots.
> In other code locations with the same pattern I didn't have to do that.
>
> Assuming '_req' passed to gr_dequeue() is located just past 'ep' on the
> heap, the check could pass even though the list searched is completely
> empty.
>
> &req->req for the head element will be an out-of-bounds pointer
> that by coincidence or heap massaging is where '_req' is located.
>
> Even if the list is empty the list_for_each_entry() macro will do:
>
> pos = list_first_entry(head, typeof(*pos), member);
>
> resolving all the macros (list_first_entry() etc) it will look like this:
>
> pos = container_of(head->next, typeof(*pos), member)
>
> Since the list is empty head->next == head and container_of() is called on something
> that is *not* actually of type gr_request.
>
> Next, the check if the end of the loop is hit is evaluated:
>
> !list_entry_is_head(pos, head, member);
>
> which will stop the loop directly before executing a single iteration.
>
> then using '&req->req' is some bogus pointer pointing just past the struct gr_ep,
> where in this case '_req' is located.
>
> The point I'm trying to make: it's probably not safe to rely on the compiler and
> that everyone is aware of this risk when adding/removing/reordering struct members.
>
>
> Signed-off-by: Jakob Koschel <[email protected]>
> ---
> drivers/usb/gadget/udc/gr_udc.c | 25 +++++++++++++++++++++++++
> drivers/usb/gadget/udc/gr_udc.h | 2 +-
> 2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/udc/gr_udc.c b/drivers/usb/gadget/udc/gr_udc.c
> index 4b35739d3695..29c662f28428 100644
> --- a/drivers/usb/gadget/udc/gr_udc.c
> +++ b/drivers/usb/gadget/udc/gr_udc.c
> @@ -1718,6 +1718,7 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> ret = -EINVAL;
> goto out;
> }
> + printk(KERN_INFO "JKL: This does print, but shouldn't");
>
> if (list_first_entry(&ep->queue, struct gr_request, queue) == req) {
> /* This request is currently being processed */
> @@ -1739,6 +1740,30 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> return ret;
> }
>
> +static int __init init_test_jkl3(void)
> +{
> + struct gr_ep *ep;
> + struct gr_udc *dev;
> + struct usb_request *_req;
> + void *buffer;
> +
> + /* assume the usb_request struct is located just after the 'ep' on the heap */
> + buffer = kzalloc(sizeof(struct gr_ep)+sizeof(struct usb_request), GFP_KERNEL);
> + ep = buffer;
> + _req = buffer+sizeof(struct gr_ep);
> +
> + /* setup to call gr_dequeue() */
> + dev = kzalloc(sizeof(struct gr_udc), GFP_KERNEL);
> + ep->dev = dev;
> + ep->dev->driver = 1;
> + INIT_LIST_HEAD(&ep->queue); /* list is empty */
> +
> + gr_dequeue(&ep->ep, _req);
> +
> + return 0;
> +}
> +__initcall(init_test_jkl3);
> +
> /* Helper for gr_set_halt and gr_set_wedge */
> static int gr_set_halt_wedge(struct usb_ep *_ep, int halt, int wedge)
> {
> diff --git a/drivers/usb/gadget/udc/gr_udc.h b/drivers/usb/gadget/udc/gr_udc.h
> index 70134239179e..14a18d5b5cf8 100644
> --- a/drivers/usb/gadget/udc/gr_udc.h
> +++ b/drivers/usb/gadget/udc/gr_udc.h
> @@ -159,7 +159,6 @@ struct gr_ep {
> };
>
> struct gr_request {
> - struct usb_request req;
> struct list_head queue;
>
> /* Chain of dma descriptors */
> @@ -171,6 +170,7 @@ struct gr_request {
> u16 oddlen; /* Size of odd length tail if buffer length is "odd" */
>
> u8 setup; /* Setup packet */
> + struct usb_request req;
> };
>
> enum gr_ep0state {
> --
> 2.25.1
So, to follow the proposed solution in this thread, the following change
is the "correct" one to make, right?
diff --git a/drivers/usb/gadget/udc/gr_udc.c b/drivers/usb/gadget/udc/gr_udc.c
index 4b35739d3695..5d65d8ad5281 100644
--- a/drivers/usb/gadget/udc/gr_udc.c
+++ b/drivers/usb/gadget/udc/gr_udc.c
@@ -1690,7 +1690,8 @@ static int gr_queue_ext(struct usb_ep *_ep, struct usb_request *_req,
/* Dequeue JUST ONE request */
static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
- struct gr_request *req;
+ struct gr_request *req = NULL;
+ struct gr_request *tmp;
struct gr_ep *ep;
struct gr_udc *dev;
int ret = 0;
@@ -1710,11 +1711,13 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
spin_lock_irqsave(&dev->lock, flags);
/* Make sure it's actually queued on this endpoint */
- list_for_each_entry(req, &ep->queue, queue) {
- if (&req->req == _req)
+ list_for_each_entry(tmp, &ep->queue, queue) {
+ if (&tmp->req == _req) {
+ req = tmp;
break;
+ }
}
- if (&req->req != _req) {
+ if (!req) {
ret = -EINVAL;
goto out;
}
On Thu, Feb 24, 2022 at 2:33 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> So, to follow the proposed solution in this thread, the following change
> is the "correct" one to make, right?
That would indeed be my preference.
If we ever are able to change
list_for_each_entry(tmp, &ep->queue, queue)...
to simply declare 'tmp' as the right type itself, then the e need to do
struct gr_request *tmp;
outside the loop goes away, and this kind of "set a variable that is
declared in the outside context" becomes the only way to do things.
Linus
On Wed, Feb 23, 2022 at 11:23:39AM -0800, Linus Torvalds wrote:
> On Wed, Feb 23, 2022 at 10:47 AM Linus Torvalds
> <[email protected]> wrote:
> >
> > Arnd - remind me, please.. Was there some other problem than just gcc-4.9?
>
> Hmm. Interesting. I decided to just try it and see for the compiler I
> have, and changing the gnu89 to gnu99 I get new warnings
> (-Werror=shift-negative-value).
>
> Very annoying. Especially since negative values are in many contexts
> actually *safer* than positive ones when used as a mask, because as
> long as the top bit is set in 'int', if the end result is then
> expanded to some wider type, the top bit stays set.
>
> Example:
>
> unsigned long mask(unsigned long x)
> { return x & (~0 << 5); }
>
> unsigned long badmask(unsigned long x)
> { return x & (~0u << 5); }
>
> One does it properly, the other is buggy.
You won't get this confusion if you write -1 instead. Not that that
helps your problem here.
The GCC documentation says (at
<https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html>):
- The results of some bitwise operations on signed integers (C90 6.3,
C99 and C11 6.5).
Bitwise operators act on the representation of the value including
both the sign and value bits, where the sign bit is considered
immediately above the highest-value value bit. Signed ‘>>’ acts on
negative numbers by sign extension.
As an extension to the C language, GCC does not use the latitude
given in C99 and C11 only to treat certain aspects of signed ‘<<’ as
undefined. However, -fsanitize=shift (and -fsanitize=undefined) will
diagnose such cases. They are also diagnosed where constant
expressions are required.
But in fact they are diagnosed more often:
===
unsigned int n;
int f0(int x) { return x & (~0 << 5); }
int f1(int x) { return x & (-1 << 5); }
int f2(int x) { return x & (~0 << n); }
int f3(int x) { return x & (-1 << n); }
===
gets a warning on every line:
===
shifty.c: In function 'f0':
shifty.c:2:32: warning: left shift of negative value [-Wshift-negative-value]
2 | int f0(int x) { return x & (~0 << 5); }
| ^~
shifty.c: In function 'f1':
shifty.c:3:32: warning: left shift of negative value [-Wshift-negative-value]
3 | int f1(int x) { return x & (-1 << 5); }
| ^~
shifty.c: In function 'f2':
shifty.c:4:32: warning: left shift of negative value [-Wshift-negative-value]
4 | int f2(int x) { return x & (~0 << n); }
| ^~
shifty.c: In function 'f3':
shifty.c:5:32: warning: left shift of negative value [-Wshift-negative-value]
5 | int f3(int x) { return x & (-1 << n); }
| ^~
===
(with -O2 -Wall -W, nothing more). No constant expression is required
in any of those cases, and in f2 and f3 the shift is not a constant
expression even.
> So this Werror=shift-negative-value warning seems to be actively
> detrimental, and I'm not seeing the reason for it. Can somebody
> explain the thinking for that stupid warning?
-Werror=*anything* is detrimental, *always*. Warnings are warnings
and errors are errors. Warnings have false positives: if not, they
should be errors instead!
> That said, we seem to only have two cases of it in the kernel, at
> least by a x86-64 allmodconfig build. So we could examine the types
> there, or we could just add '-Wno-shift-negative-value".
Yes.
The only reason the warning exists is because it is undefined behaviour
(not implementation-defined or anything). The reason it is that in the
standard is that it is hard to implement and even describe for machines
that are not two's complement. However relevant that is today :-)
Segher
On Sat, Feb 26, 2022 at 1:42 PM Segher Boessenkool
<[email protected]> wrote:
> On Wed, Feb 23, 2022 at 11:23:39AM -0800, Linus Torvalds wrote:
> > That said, we seem to only have two cases of it in the kernel, at
> > least by a x86-64 allmodconfig build. So we could examine the types
> > there, or we could just add '-Wno-shift-negative-value".
>
> Yes.
>
> The only reason the warning exists is because it is undefined behaviour
> (not implementation-defined or anything). The reason it is that in the
> standard is that it is hard to implement and even describe for machines
> that are not two's complement. However relevant that is today :-)
Could gcc follow the clang behavior then and skip the warning and
sanitizer for this case when -fno-strict-overflow or -fwrapv are used?
Arnd
On Sat, Feb 26, 2022 at 2:14 PM Arnd Bergmann <[email protected]> wrote:
>
> Could gcc follow the clang behavior then and skip the warning and
> sanitizer for this case when -fno-strict-overflow or -fwrapv are used?
Well, for the kernel, that horse has already left the barn, and we'd
have to use -Wno-shift-negative-value anyway.
But yes, from a sanity standpoint, it would be good to shut that
warning up automatically if compiling for a 2's complement machine (ie
"all of them") with -fwrapv.
Considering that gcc doesn't support any non-2's-complement machines
anyway afaik, and that the C standards people are also fixing the
standard, and gcc has never done anything odd in this area in the
first place, I think the warning is probably best removed entirely.
But we'll have to do it manually for the existing situation.
Linus
On Sat, Feb 26, 2022 at 11:14:15PM +0100, Arnd Bergmann wrote:
> On Sat, Feb 26, 2022 at 1:42 PM Segher Boessenkool
> <[email protected]> wrote:
> > On Wed, Feb 23, 2022 at 11:23:39AM -0800, Linus Torvalds wrote:
>
> > > That said, we seem to only have two cases of it in the kernel, at
> > > least by a x86-64 allmodconfig build. So we could examine the types
> > > there, or we could just add '-Wno-shift-negative-value".
> >
> > Yes.
> >
> > The only reason the warning exists is because it is undefined behaviour
> > (not implementation-defined or anything). The reason it is that in the
> > standard is that it is hard to implement and even describe for machines
> > that are not two's complement. However relevant that is today :-)
>
> Could gcc follow the clang behavior then and skip the warning and
> sanitizer for this case when -fno-strict-overflow or -fwrapv are used?
As I said, we have this implementation choice documented as
As an extension to the C language, GCC does not use the latitude
given in C99 and C11 only to treat certain aspects of signed '<<'
as undefined. However, '-fsanitize=shift' (and
'-fsanitize=undefined') will diagnose such cases. They are also
diagnosed where constant expressions are required.
but that is not at all what we implement currently, we warn much more
often. Constant expressions are required only in a few places (#if
condition, bitfield length, (non-variable) array length, enumeration
declarations, _Alignas, _Static_assert, case labels, most initialisers);
not places you will see code like this problem normally.
So imo we should just never do this by default, not just if the nasty
-fwrapv or nastier -fno-strict-overflow is used, just like we suggest
in our own documentation. The only valid reason -Wshift-negative-value
is in -Wextra is it warns for situations that always are undefined
behaviour (even if not in GCC).
Could you open a GCC PR for this? The current situation is quite
suboptimal, and what we document as our implementation choice is much
more useful!
Segher
On Sat, Feb 26, 2022 at 03:03:09PM -0800, Linus Torvalds wrote:
> On Sat, Feb 26, 2022 at 2:14 PM Arnd Bergmann <[email protected]> wrote:
> >
> > Could gcc follow the clang behavior then and skip the warning and
> > sanitizer for this case when -fno-strict-overflow or -fwrapv are used?
>
> Well, for the kernel, that horse has already left the barn, and we'd
> have to use -Wno-shift-negative-value anyway.
>
> But yes, from a sanity standpoint, it would be good to shut that
> warning up automatically if compiling for a 2's complement machine (ie
> "all of them") with -fwrapv.
>
> Considering that gcc doesn't support any non-2's-complement machines
> anyway afaik,
* 'Whether signed integer types are represented using sign and
magnitude, two's complement, or one's complement, and whether the
extraordinary value is a trap representation or an ordinary value
(C99 and C11 6.2.6.2).'
GCC supports only two's complement integer types, and all bit
patterns are ordinary values.
> and that the C standards people are also fixing the
> standard, and gcc has never done anything odd in this area in the
> first place, I think the warning is probably best removed entirely.
Well, not removed, it correctly identifies (formally) undefined
behaviour after all; but I agree it should not be in -Wextra.
-Wall should include the warnings that have a very good balance for
usefulness, number of false postives, seriousness of problems found.
-Wextra is exactly the same conditions, just a slightly lower bar.
-Wall should be useful for everyone. -Wall -W should be good for most
people.
> But we'll have to do it manually for the existing situation.
Yes, sorry about that :-/
Segher
From: Segher Boessenkool
> Sent: 27 February 2022 01:10
>
> On Sat, Feb 26, 2022 at 11:14:15PM +0100, Arnd Bergmann wrote:
> > On Sat, Feb 26, 2022 at 1:42 PM Segher Boessenkool
> > <[email protected]> wrote:
> > > On Wed, Feb 23, 2022 at 11:23:39AM -0800, Linus Torvalds wrote:
> >
> > >
> > > The only reason the warning exists is because it is undefined behaviour
> > > (not implementation-defined or anything). The reason it is that in the
> > > standard is that it is hard to implement and even describe for machines
> > > that are not two's complement. However relevant that is today :-)
I thought only right shifts of negative values were 'undefined'.
And that was to allow cpu that only had logical shift right
(ie ones that didn't propagate the sign) to be conformant.
I wonder when the last cpu like that was?
Quite why the standards keeps using the term 'undefined behaviour'
beats me - there ought to be something for 'undefined value'.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Sun, Feb 27, 2022 at 07:10:45AM +0000, David Laight wrote:
> From: Segher Boessenkool
> > Sent: 27 February 2022 01:10
> > On Sat, Feb 26, 2022 at 11:14:15PM +0100, Arnd Bergmann wrote:
> > > On Sat, Feb 26, 2022 at 1:42 PM Segher Boessenkool
> > > <[email protected]> wrote:
> > > > The only reason the warning exists is because it is undefined behaviour
> > > > (not implementation-defined or anything). The reason it is that in the
> > > > standard is that it is hard to implement and even describe for machines
> > > > that are not two's complement. However relevant that is today :-)
>
> I thought only right shifts of negative values were 'undefined'.
All shifts by a negative amount, or an amount greater or equal to the
width of the first operand (after the integer promotions).
Right shifts of a negative value.
Left shifts of a negative value where E1*2**E2 is not expressable in the
result type.
C90 (aka C89) had those right shifts merely implementation-defined
behaviour, and the left shifts perfectly well-defined.
> And that was to allow cpu that only had logical shift right
> (ie ones that didn't propagate the sign) to be conformant.
The C99 rationale says
The C89 Committee affirmed the freedom in implementation granted by
K&R in not requiring the signed right shift operation to sign extend,
since such a requirement might slow down fast code and since the
usefulness of sign extended shifts is marginal. (Shifting a negative
two’s-complement integer arithmetically right one place is not the
same as dividing by two!)
> I wonder when the last cpu like that was?
There still are one-off cores without such an instruction. If you have
right shifts at all (if you have shifts at all!) it quickly becomes
apparent what a hassle it is not to have an SRA/ASR/SAR instruction, and
it is easy to implement, so :-)
The last widely spread ones' complement machine was the 6600 I guess,
which disappeared somewhere in the 80's. Sign-magnitude machines are
still made: all FP is like that, and some (simple, embedded, etc.)
machines have no separate integer register set. C is available for most
such fringe CPUs :-)
> Quite why the standards keeps using the term 'undefined behaviour'
> beats me - there ought to be something for 'undefined value'.
It is pretty much impossible to not have *some* undefined behaviour.
How will you define dividing by zero so that its behaviour is reasonable
for every program, for example? Invoking an error handler at runtime
has most of the same unwanted effects, except is is never silent. You
can get that via UBSAN for example. Defining some arbitrary value as
the "correct" answer when that is not at all obvious *does* give silent,
unexpected results.
C does have "unspecified value"s and "unspecified behaviour". It
requires the implementation to document the choice made here. At least
some effort was made not to have undefined behaviour everywhere.
Perhaps C does have the concept you are after in 6.3.2/2, where it talks
about using uninitialised objects? Of course, using an uninitialised
object is undefined behaviour :-P
C has much more undefined behaviours than most other languages. On one
hand there was no clear, formal definition of the language (and
testsuites for it etc.) before it became popular. On the other hand, it
was implemented on widely different architectures, back in the days when
there was a lot more variety in implementation choices than there is
now. When the language was standardised (and all the way to this day)
the sentiment was to not unnecessarily break existing implementations.
Segher
On Sun, Feb 27, 2022 at 2:09 AM Segher Boessenkool
<[email protected]> wrote:
>
> So imo we should just never do this by default, not just if the nasty
> -fwrapv or nastier -fno-strict-overflow is used, just like we suggest
> in our own documentation. The only valid reason -Wshift-negative-value
> is in -Wextra is it warns for situations that always are undefined
> behaviour (even if not in GCC).
Ok, I just realized that this is specific to the i915 driver because
that, unlike
most of the kernel builds with -Wextra by default. -Wextra is enabled when
users ask for a 'make W=1' build in linux, and i915 is one of just three
drivers that enable an equivalent set of warnings, the other ones
being greybus and btrfs.
This means to work around the extra warnings, we also just need to disable
it in the W=1 part of scripts/Makefile.extrawarn, as well as the three drivers
that copy those options, but not the default warnings that don't include them.
> Could you open a GCC PR for this? The current situation is quite
> suboptimal, and what we document as our implementation choice is much
> more useful!
I hope I managed to capture the issue in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104711
Arnd
On Sun, Feb 27, 2022 at 1:09 PM Segher Boessenkool
<[email protected]> wrote:
>
> How will you define dividing by zero so that its behaviour is reasonable
> for every program, for example?
The solution is to let the developer specify what they need to happen.
That choice should include the unsafe possibility (i.e. unchecked),
because sometimes that is precisely what we need.
> Invoking an error handler at runtime
> has most of the same unwanted effects, except is is never silent. You
It may not be what it is needed in some cases (thus the necessity to
be able to choose), but at least one can predict what happens and
different compilers, versions, flags, inputs, etc. would agree.
Cheers,
Miguel
On Sun, Feb 27, 2022 at 10:28:41PM +0100, Arnd Bergmann wrote:
> On Sun, Feb 27, 2022 at 2:09 AM Segher Boessenkool
> <[email protected]> wrote:
> >
> > So imo we should just never do this by default, not just if the nasty
> > -fwrapv or nastier -fno-strict-overflow is used, just like we suggest
> > in our own documentation. The only valid reason -Wshift-negative-value
> > is in -Wextra is it warns for situations that always are undefined
> > behaviour (even if not in GCC).
>
> Ok, I just realized that this is specific to the i915 driver because
> that, unlike
> most of the kernel builds with -Wextra by default. -Wextra is enabled when
> users ask for a 'make W=1' build in linux, and i915 is one of just three
> drivers that enable an equivalent set of warnings, the other ones
> being greybus and btrfs.
>
> This means to work around the extra warnings, we also just need to disable
> it in the W=1 part of scripts/Makefile.extrawarn, as well as the three drivers
> that copy those options, but not the default warnings that don't include them.
Ah good, all of the workaround in one simple place, neat.
> > Could you open a GCC PR for this? The current situation is quite
> > suboptimal, and what we document as our implementation choice is much
> > more useful!
>
> I hope I managed to capture the issue in
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104711
That looks fine. Thank you!
(I attached the testcase to the bug itself, we prefer it that way, maybe
godbolt will go away some day, who knows.)
Segher
On Sun, Feb 27, 2022 at 07:09:03PM +0100, Miguel Ojeda wrote:
> On Sun, Feb 27, 2022 at 1:09 PM Segher Boessenkool
> <[email protected]> wrote:
> >
> > How will you define dividing by zero so that its behaviour is reasonable
> > for every program, for example?
>
> The solution is to let the developer specify what they need to happen.
> That choice should include the unsafe possibility (i.e. unchecked),
> because sometimes that is precisely what we need.
Requiring to annotate every place that has UB (or *can* have UB!) by the
user is even less friendly than having so much UB is already :-(
I don't see how you will fit this into the C syntax, btw?
> > Invoking an error handler at runtime
> > has most of the same unwanted effects, except is is never silent. You
>
> It may not be what it is needed in some cases (thus the necessity to
> be able to choose), but at least one can predict what happens and
> different compilers, versions, flags, inputs, etc. would agree.
You need a VM like Java's to get even *close* to that. This is not the
C target: it is slower than wanted/expected, it is hosted instead of
embedded, and it comes with a whole host of issues of its own. One of
the strengths of C is its tiny runtime, a few kB is a lot already!
I completely agree that if you design a new "systems" language, you want
to have much less undefined behaviour than C has. But it is self-
delusion to think you can eradicate all (or even most).
And there are much bigger problems in any case! If you think that if
programmers could no longer write programs that invoke undefined
behaviour they will write much better programs, programs with fewer
serious functionality or security problems, even just a factor of two
better, well...
Segher