Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

Why is (2 < 2) true? Is it a gcc bug?

12 views
Skip to first unread message

Dorau, Lukasz

unread,
Jan 17, 2014, 8:40:01 AM1/17/14
to
Hi

My story is very simply...
I applied the following patch:

diff --git a/drivers/scsi/isci/init.c b/drivers/scsi/isci/init.c
--- a/drivers/scsi/isci/init.c
+++ b/drivers/scsi/isci/init.c
@@ -698,8 +698,11 @@ static int isci_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
if (err)
goto err_host_alloc;

- for_each_isci_host(i, isci_host, pdev)
+ for_each_isci_host(i, isci_host, pdev) {
+ pr_err("(%d < %d) == %d\n",\
+ i, SCI_MAX_CONTROLLERS, (i < SCI_MAX_CONTROLLERS));
scsi_scan_host(to_shost(isci_host));
+ }

return 0;

--
1.8.3.1

Then I issued the command 'modprobe isci' on platform with two SCU controllers (Patsburg D or T chipset)
and received the following, very strange, output:

(0 < 2) == 1
(1 < 2) == 1
(2 < 2) == 1

Can anyone explain why (2 < 2) is true? Is it a gcc bug?

(The kernel was compiled using gcc version 4.8.2.)

Lukasz

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Dorau, Lukasz

unread,
Jan 17, 2014, 9:00:02 AM1/17/14
to
Some additional information:

The loop 'for' in macro ' for_each_isci_host ' defined as (drivers/scsi/isci/host.h:313):

#define for_each_isci_host(id, ihost, pdev) \
for (id = 0, ihost = to_pci_info(pdev)->hosts[id]; \
id < ARRAY_SIZE(to_pci_info(pdev)->hosts) && ihost; \
ihost = to_pci_info(pdev)->hosts[++id])

should be executed only for i = 0 and 1, because:
ARRAY_SIZE(to_pci_info(pdev)->hosts) = SCI_MAX_CONTROLLERS = 2

but it was executed also for i=2 regardless the above loop's end condition.

Richard Weinberger

unread,
Jan 17, 2014, 9:00:02 AM1/17/14
to
Can you reproduce this using a standalone test?
I.e:
#include <assert.h>

int main()
{
assert(2 < 2 != 1);

return 0;
}

--
Thanks,
//richard

Dorau, Lukasz

unread,
Jan 17, 2014, 10:00:02 AM1/17/14
to
On Friday, January 17, 2014 2:58 PM Richard Weinberger <richard.w...@gmail.com> wrote:
>
> Can you reproduce this using a standalone test?
> I.e:
> #include <assert.h>
>
> int main()
> {
> assert(2 < 2 != 1);
>
> return 0;
> }
>

No, I can't of course.

Lukasz

Sebastian Riemer

unread,
Jan 17, 2014, 11:50:01 AM1/17/14
to
to_pci_info() can return NULL in dev_get_drvdata(). The use of
ARRAY_SIZE() is inappropriate.

#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
__must_be_array(arr))

#define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))

#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))

I would say that this was supposed to trigger a build error but it
didn't and added 1 to the loop end condition.

Can you test putting SCI_MAX_CONTROLLERS to the loop end condition, please?

Cheers,
Sebastian

Dorau, Lukasz

unread,
Jan 17, 2014, 12:10:02 PM1/17/14
to
On Friday, January 17, 2014 5:40 PM Sebastian Riemer <sebastia...@profitbricks.com> wrote:
> On 17.01.2014 14:55, Dorau, Lukasz wrote:
> >
> > Some additional information:
> >
> > The loop 'for' in macro ' for_each_isci_host ' defined as
> (drivers/scsi/isci/host.h:313):
> >
> > #define for_each_isci_host(id, ihost, pdev) \
> > for (id = 0, ihost = to_pci_info(pdev)->hosts[id]; \
> > id < ARRAY_SIZE(to_pci_info(pdev)->hosts) && ihost; \
> > ihost = to_pci_info(pdev)->hosts[++id])
> >
> > should be executed only for i = 0 and 1, because:
> > ARRAY_SIZE(to_pci_info(pdev)->hosts) = SCI_MAX_CONTROLLERS = 2
> >
> > but it was executed also for i=2 regardless the above loop's end condition.
>
> to_pci_info() can return NULL in dev_get_drvdata(). The use of
> ARRAY_SIZE() is inappropriate.
>
> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> __must_be_array(arr))
>
> #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
>
> #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
>
> I would say that this was supposed to trigger a build error but it
> didn't and added 1 to the loop end condition.
>
> Can you test putting SCI_MAX_CONTROLLERS to the loop end condition, please?
>

Replacing 'ARRAY_SIZE(to_pci_info(pdev)->hosts)' with 'SCI_MAX_CONTROLLERS' in the definition of the ' for_each_isci_host ' macro does not help. I have checked it.
The following patch helps:
http://marc.info/?l=linux-scsi&m=138987823011697&w=2

Lukasz

Alexei Starovoitov

unread,
Jan 17, 2014, 1:00:02 PM1/17/14
to
gcc sees that i < array_size is the same as i < 2 as part of loop condition, so
it optimizes (i < sci_max_controllers) into constant 1.
and emits printk like:
printk ("\13(%d < %d) == %d\n", i_382, 2, 1);

> (The kernel was compiled using gcc version 4.8.2.)

it actually looks to be gcc 4.8 bug.
Can you try gcc 4.7 ?

gcc 4.7 compiles your loop into the following:
<bb 74>:
# i_382 = PHI <0(73), i_73(74)>
# isci_host_148 = PHI <isci_host_63(73), isci_host_74(74)>
printk ("\13(%d < %d) == %d\n", i_382, 2, 1);
D.43295_70 = MEM[(struct isci_host *)isci_host_148 + 18632B];
# DEBUG D#6 => isci_host_148
# DEBUG ihost s=> ihost
scsi_scan_host (D.43295_70);
# DEBUG pdev => pdev_17(D)
# DEBUG pdev => pdev_17(D)
D.43629_353 = dev_get_drvdata (D.42809_20);
i_73 = i_382 + 1;
# DEBUG i => i_73
isci_host_74 = MEM[(struct isci_pci_info *)D.43629_353].hosts[i_73];
# DEBUG isci_host => isci_host_74
# DEBUG isci_host => isci_host_74
# DEBUG i => i_73
i.9_79 = (unsigned int) i_73;
D.42849_65 = i.9_79 <= 1;
D.42850_66 = isci_host_74 != 0B;
D.42851_67 = D.42850_66 & D.42849_65;
if (D.42851_67 != 0)
goto <bb 74>;
else
goto <bb 77>;

which looks correct to me.

while gcc 4.8.2 into:
<bb 92>:
# i_73 = PHI <i_82(93), 0(91)>
# isci_host_274 = PHI <isci_host_83(93), isci_host_71(91)>
# DEBUG isci_host => isci_host_274
# DEBUG i => i_73
printk ("\13(%d < %d) == %d\n", i_73, 2, 1);
_79 = MEM[(struct isci_host *)isci_host_274 + 18632B];
# DEBUG D#6 => isci_host_274
# DEBUG ihost => D#6
scsi_scan_host (_79);
# DEBUG pdev => pdev_26(D)
# DEBUG pdev => pdev_26(D)
_97 = dev_get_drvdata (_29);
i_82 = i_73 + 1;
# DEBUG i => i_82
isci_host_83 = MEM[(struct isci_pci_info *)_97].hosts[i_82];
# DEBUG isci_host => isci_host_83
# DEBUG isci_host => isci_host_83
# DEBUG i => i_82
if (isci_host_83 != 0B)
goto <bb 93>;
else
goto <bb 90>;

<bb 93>:
goto <bb 92>;

in case of gcc4.8 the i<=1 comparison got optimized out and only
isci_host !=0 is left,
which looks incorrect.

Alexei Starovoitov

unread,
Jan 17, 2014, 3:00:03 PM1/17/14
to
It is interesting GCC 4.8 bug,
since it seems to expose issues in two compiler passes.

here is test case:

struct isci_host;
struct isci_orom;

struct isci_pci_info {
struct isci_host *hosts[2];
struct isci_orom *orom;
} v = {{(struct isci_host *)1,(struct isci_host *)1}, 0};

int printf(const char *fmt, ...);

int isci_pci_probe()
{
int i;
struct isci_host *isci_host;

for (i = 0, isci_host = v.hosts[i];
i < 2 && isci_host;
isci_host = v.hosts[++i]) {
printf("(%d < %d) == %d\n", i, 2, (i < 2));
}

return 0;
}

int main()
{
isci_pci_probe();
}

$ gcc bug.c
$./a.out
0 < 2) == 1
(1 < 2) == 1
$ gcc bug.c -O2
$ ./a.out
(0 < 2) == 1
(1 < 2) == 1
Segmentation fault (core dumped)

workaround:

disable Value Range Propagation pass:
-fdisable-tree-vrp1 -fdisable-tree-vrp2

and complete unroll pass:
-fdisable-tree-cunrolli

Andi Kleen

unread,
Jan 17, 2014, 3:30:04 PM1/17/14
to
Alexei Starovoitov <alexei.st...@gmail.com> writes:
>
> disable Value Range Propagation pass:
> -fdisable-tree-vrp1 -fdisable-tree-vrp2
>
> and complete unroll pass:
> -fdisable-tree-cunrolli

Can you file a gcc bug with test case?

-Andi

--
a...@linux.intel.com -- Speaking for myself only

Markus Trippelsdorf

unread,
Jan 17, 2014, 4:10:03 PM1/17/14
to
Your testcase is invalid:

markus@x4 tmp % clang -fsanitize=undefined -Wall -Wextra -O2 bug.c
markus@x4 tmp % ./a.out
(0 < 2) == 1
(1 < 2) == 1
bug.c:16:20: runtime error: index 2 out of bounds for type 'struct isci_host *[2]'

As Jakub Jelinek said on IRC, changing the loop to e.g.:

for (i = 0;
i < 2 && (isci_host = v.hosts[i]);
i++) {

fixes the issue.

--
Markus

Alexei Starovoitov

unread,
Jan 17, 2014, 4:50:03 PM1/17/14
to
testcase was reduced from drivers/scsi/isci/host.h written back in
2011 (commit b329aff107)
#define for_each_isci_host(id, ihost, pdev) \
for (id = 0, ihost = to_pci_info(pdev)->hosts[id]; \
id < ARRAY_SIZE(to_pci_info(pdev)->hosts) && ihost; \
ihost = to_pci_info(pdev)->hosts[++id])

yes, it does access 3rd element of 2 element array and will read junk.

C standard says the behavior is undefined and comes handy in compiler defense,
but in this case compiler has obviously all the information to make
right decision
instead of misoptimizing the code.
So yes, the loop is erroneous, non-portable, etc, but gcc can be smarter.

Dorau, Lukasz

unread,
Jan 18, 2014, 6:40:02 AM1/18/14
to
Thank you for explanation!

Alexei,

Will you file a gcc bug for this case?

Thanks,
Lukasz

Alexei Starovoitov

unread,
Jan 20, 2014, 2:50:02 PM1/20/14
to
sure. filed for the record:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59892
Closed as invalid by Markus already.
0 new messages