I noticed a build error in some randconfig builds in the zl10353 driver:
dvb-frontends/zl10353.c:138: undefined reference to `____ilog2_NaN'
dvb-frontends/zl10353.c:138: undefined reference to `__aeabi_uldivmod'
The problem can be tracked down to the use of -fprofile-arcs (using
CONFIG_GCOV_PROFILE_ALL) in combination with CONFIG_PROFILE_ALL_BRANCHES
on gcc version 4.9 or higher, when it fails to reliably optimize
constant expressions.
Using div_u64() instead of do_div() makes the code slightly more
readable by both humans and by gcc, which gives the compiler enough
of a break to figure it all out.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/media/dvb-frontends/zl10353.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
Em Fri, 12 Feb 2016 15:27:18 +0100
Arnd Bergmann [off-list ref] escreveu:
I noticed a build error in some randconfig builds in the zl10353 driver:
dvb-frontends/zl10353.c:138: undefined reference to `____ilog2_NaN'
dvb-frontends/zl10353.c:138: undefined reference to `__aeabi_uldivmod'
The problem can be tracked down to the use of -fprofile-arcs (using
CONFIG_GCOV_PROFILE_ALL) in combination with CONFIG_PROFILE_ALL_BRANCHES
on gcc version 4.9 or higher, when it fails to reliably optimize
constant expressions.
Using div_u64() instead of do_div() makes the code slightly more
readable by both humans and by gcc, which gives the compiler enough
of a break to figure it all out.
I'm not against this patch, but we have 94 occurrences of do_div()
just at the media subsystem. If this is failing here, it would likely
fail with other drivers. So, I guess we should either fix do_div() or
convert all such occurrences to do_div64().
On Friday 12 February 2016 14:32:20 Mauro Carvalho Chehab wrote:
Em Fri, 12 Feb 2016 15:27:18 +0100
Arnd Bergmann [off-list ref] escreveu:
quoted
I noticed a build error in some randconfig builds in the zl10353 driver:
dvb-frontends/zl10353.c:138: undefined reference to `____ilog2_NaN'
dvb-frontends/zl10353.c:138: undefined reference to `__aeabi_uldivmod'
The problem can be tracked down to the use of -fprofile-arcs (using
CONFIG_GCOV_PROFILE_ALL) in combination with CONFIG_PROFILE_ALL_BRANCHES
on gcc version 4.9 or higher, when it fails to reliably optimize
constant expressions.
Using div_u64() instead of do_div() makes the code slightly more
readable by both humans and by gcc, which gives the compiler enough
of a break to figure it all out.
I'm not against this patch, but we have 94 occurrences of do_div()
just at the media subsystem. If this is failing here, it would likely
fail with other drivers. So, I guess we should either fix do_div() or
convert all such occurrences to do_div64().
I agree that it's possible that the same problem exists elsewhere, but this is
the only one that I ever saw (in five ranconfig builds out of 8035 last week).
I also tried changing do_div() to be an inline function with just a small
macro wrapper around it for the odd calling conventions, which also made this
error go away. I would assume that Nico had a good reason for doing do_div()
the way he did. In some other files, I saw the object code grow by a few
instructions, but the examples I looked at were otherwise identical.
I can imagine that there might be cases where the constant-argument optimization
of do_div fails when we go through an inline function in some combination
of Kconfig options and compiler version, though I don't think that was
the case here.
Nico, any other thoughts on this?
Arnd
From: Nicolas Pitre <hidden> Date: 2016-02-12 18:21:39
On Fri, 12 Feb 2016, Arnd Bergmann wrote:
On Friday 12 February 2016 14:32:20 Mauro Carvalho Chehab wrote:
quoted
Em Fri, 12 Feb 2016 15:27:18 +0100
Arnd Bergmann [off-list ref] escreveu:
quoted
I noticed a build error in some randconfig builds in the zl10353 driver:
dvb-frontends/zl10353.c:138: undefined reference to `____ilog2_NaN'
dvb-frontends/zl10353.c:138: undefined reference to `__aeabi_uldivmod'
The problem can be tracked down to the use of -fprofile-arcs (using
CONFIG_GCOV_PROFILE_ALL) in combination with CONFIG_PROFILE_ALL_BRANCHES
on gcc version 4.9 or higher, when it fails to reliably optimize
constant expressions.
Using div_u64() instead of do_div() makes the code slightly more
readable by both humans and by gcc, which gives the compiler enough
of a break to figure it all out.
I'm not against this patch, but we have 94 occurrences of do_div()
just at the media subsystem. If this is failing here, it would likely
fail with other drivers. So, I guess we should either fix do_div() or
convert all such occurrences to do_div64().
I agree that it's possible that the same problem exists elsewhere, but this is
the only one that I ever saw (in five ranconfig builds out of 8035 last week).
I also tried changing do_div() to be an inline function with just a small
macro wrapper around it for the odd calling conventions, which also made this
error go away. I would assume that Nico had a good reason for doing do_div()
the way he did.
The do_div() calling convention predates my work on it. I assume it was
originally done this way to better map onto the x86 instruction.
In some other files, I saw the object code grow by a few
instructions, but the examples I looked at were otherwise identical.
I can imagine that there might be cases where the constant-argument optimization
of do_div fails when we go through an inline function in some combination
of Kconfig options and compiler version, though I don't think that was
the case here.
What could be tried is to turn __div64_const32() into a static inline
and see if that makes a difference with those gcc versions we currently
accept.
On Friday 12 February 2016 13:21:33 Nicolas Pitre wrote:
On Fri, 12 Feb 2016, Arnd Bergmann wrote:
quoted
On Friday 12 February 2016 14:32:20 Mauro Carvalho Chehab wrote:
quoted
Em Fri, 12 Feb 2016 15:27:18 +0100
Arnd Bergmann [off-list ref] escreveu:
quoted
I noticed a build error in some randconfig builds in the zl10353 driver:
dvb-frontends/zl10353.c:138: undefined reference to `____ilog2_NaN'
dvb-frontends/zl10353.c:138: undefined reference to `__aeabi_uldivmod'
The problem can be tracked down to the use of -fprofile-arcs (using
CONFIG_GCOV_PROFILE_ALL) in combination with CONFIG_PROFILE_ALL_BRANCHES
on gcc version 4.9 or higher, when it fails to reliably optimize
constant expressions.
Using div_u64() instead of do_div() makes the code slightly more
readable by both humans and by gcc, which gives the compiler enough
of a break to figure it all out.
I'm not against this patch, but we have 94 occurrences of do_div()
just at the media subsystem. If this is failing here, it would likely
fail with other drivers. So, I guess we should either fix do_div() or
convert all such occurrences to do_div64().
I agree that it's possible that the same problem exists elsewhere, but this is
the only one that I ever saw (in five ranconfig builds out of 8035 last week).
I also tried changing do_div() to be an inline function with just a small
macro wrapper around it for the odd calling conventions, which also made this
error go away. I would assume that Nico had a good reason for doing do_div()
the way he did.
The do_div() calling convention predates my work on it. I assume it was
originally done this way to better map onto the x86 instruction.
Right, this goes back to the dawn of time.
quoted
In some other files, I saw the object code grow by a few
instructions, but the examples I looked at were otherwise identical.
I can imagine that there might be cases where the constant-argument optimization
of do_div fails when we go through an inline function in some combination
of Kconfig options and compiler version, though I don't think that was
the case here.
What could be tried is to turn __div64_const32() into a static inline
and see if that makes a difference with those gcc versions we currently
accept.
I have a fairly recent gcc, but I also never got around to submit
it properly.
However, I did stumble over an older patch I did now, which I could
not remember what it was good for. It does fix the problem, and
it seems to be a better solution.
Arnd
I have a fairly recent gcc, but I also never got around to submit
it properly.
However, I did stumble over an older patch I did now, which I could
not remember what it was good for. It does fix the problem, and
it seems to be a better solution.
WTF?
Hmmm... it apparently doesn't fix it if I apply this change to the gcc
test case.
I have a fairly recent gcc, but I also never got around to submit
it properly.
However, I did stumble over an older patch I did now, which I could
not remember what it was good for. It does fix the problem, and
it seems to be a better solution.
WTF?
Even better, it also fixes this one:
drivers/mtd/chips/cfi_cmdset_0020.c: In function 'cfi_staa_write_buffers':
drivers/mtd/chips/cfi_cmdset_0020.c:651:1: error: the frame size of 1064 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
I have not even looked what that is, I only saw show up the other day.
Arnd
On 12 February 2016 at 22:01, Arnd Bergmann [off-list ref] wrote:
quoted hunk
On Friday 12 February 2016 13:21:33 Nicolas Pitre wrote:
quoted
On Fri, 12 Feb 2016, Arnd Bergmann wrote:
quoted
On Friday 12 February 2016 14:32:20 Mauro Carvalho Chehab wrote:
quoted
Em Fri, 12 Feb 2016 15:27:18 +0100
Arnd Bergmann [off-list ref] escreveu:
quoted
I noticed a build error in some randconfig builds in the zl10353 driver:
dvb-frontends/zl10353.c:138: undefined reference to `____ilog2_NaN'
dvb-frontends/zl10353.c:138: undefined reference to `__aeabi_uldivmod'
The problem can be tracked down to the use of -fprofile-arcs (using
CONFIG_GCOV_PROFILE_ALL) in combination with CONFIG_PROFILE_ALL_BRANCHES
on gcc version 4.9 or higher, when it fails to reliably optimize
constant expressions.
Using div_u64() instead of do_div() makes the code slightly more
readable by both humans and by gcc, which gives the compiler enough
of a break to figure it all out.
I'm not against this patch, but we have 94 occurrences of do_div()
just at the media subsystem. If this is failing here, it would likely
fail with other drivers. So, I guess we should either fix do_div() or
convert all such occurrences to do_div64().
I agree that it's possible that the same problem exists elsewhere, but this is
the only one that I ever saw (in five ranconfig builds out of 8035 last week).
I also tried changing do_div() to be an inline function with just a small
macro wrapper around it for the odd calling conventions, which also made this
error go away. I would assume that Nico had a good reason for doing do_div()
the way he did.
The do_div() calling convention predates my work on it. I assume it was
originally done this way to better map onto the x86 instruction.
Right, this goes back to the dawn of time.
quoted
quoted
In some other files, I saw the object code grow by a few
instructions, but the examples I looked at were otherwise identical.
I can imagine that there might be cases where the constant-argument optimization
of do_div fails when we go through an inline function in some combination
of Kconfig options and compiler version, though I don't think that was
the case here.
What could be tried is to turn __div64_const32() into a static inline
and see if that makes a difference with those gcc versions we currently
accept.
I have a fairly recent gcc, but I also never got around to submit
it properly.
However, I did stumble over an older patch I did now, which I could
not remember what it was good for. It does fix the problem, and
it seems to be a better solution.
Arnd
I remember seeing this patch, but I don't remember the exact context.
But when you think about it, !!cond can be a build time constant even
if cond is not, as long as you can prove statically that cond != 0. So
I think this change is obviously correct, and an improvement since it
will remove the profiling overhead of branches that are not true
branches in the first place.
From: Nicolas Pitre <hidden> Date: 2016-02-13 21:57:46
On Sat, 13 Feb 2016, Ard Biesheuvel wrote:
On 12 February 2016 at 22:01, Arnd Bergmann [off-list ref] wrote:
quoted
However, I did stumble over an older patch I did now, which I could
not remember what it was good for. It does fix the problem, and
it seems to be a better solution.
Arnd
I remember seeing this patch, but I don't remember the exact context.
But when you think about it, !!cond can be a build time constant even
if cond is not, as long as you can prove statically that cond != 0. So
You're right. I just tested it and to my surprise gcc is smart enough
to figure that case out.
I think this change is obviously correct, and an improvement since it
will remove the profiling overhead of branches that are not true
branches in the first place.
On 13 February 2016 at 22:57, Nicolas Pitre [off-list ref] wrote:
On Sat, 13 Feb 2016, Ard Biesheuvel wrote:
quoted
On 12 February 2016 at 22:01, Arnd Bergmann [off-list ref] wrote:
quoted
However, I did stumble over an older patch I did now, which I could
not remember what it was good for. It does fix the problem, and
it seems to be a better solution.
Arnd
I remember seeing this patch, but I don't remember the exact context.
But when you think about it, !!cond can be a build time constant even
if cond is not, as long as you can prove statically that cond != 0. So
You're right. I just tested it and to my surprise gcc is smart enough
to figure that case out.
quoted
I think this change is obviously correct, and an improvement since it
will remove the profiling overhead of branches that are not true
branches in the first place.
Indeed.
... and perhaps we should not evaluate cond twice either?
From: Nicolas Pitre <hidden> Date: 2016-02-14 16:52:15
On Sun, 14 Feb 2016, Ard Biesheuvel wrote:
On 13 February 2016 at 22:57, Nicolas Pitre [off-list ref] wrote:
quoted
On Sat, 13 Feb 2016, Ard Biesheuvel wrote:
quoted
On 12 February 2016 at 22:01, Arnd Bergmann [off-list ref] wrote:
quoted
However, I did stumble over an older patch I did now, which I could
not remember what it was good for. It does fix the problem, and
it seems to be a better solution.
Arnd
I remember seeing this patch, but I don't remember the exact context.
But when you think about it, !!cond can be a build time constant even
if cond is not, as long as you can prove statically that cond != 0. So
You're right. I just tested it and to my surprise gcc is smart enough
to figure that case out.
quoted
I think this change is obviously correct, and an improvement since it
will remove the profiling overhead of branches that are not true
branches in the first place.
Indeed.
... and perhaps we should not evaluate cond twice either?
It is not. The value of the argument to __builtin_constant_p() is not
itself evaluated and therefore does not produce side effects.
Nicolas
On 14 February 2016 at 17:52, Nicolas Pitre [off-list ref] wrote:
On Sun, 14 Feb 2016, Ard Biesheuvel wrote:
quoted
On 13 February 2016 at 22:57, Nicolas Pitre [off-list ref] wrote:
quoted
On Sat, 13 Feb 2016, Ard Biesheuvel wrote:
quoted
On 12 February 2016 at 22:01, Arnd Bergmann [off-list ref] wrote:
quoted
However, I did stumble over an older patch I did now, which I could
not remember what it was good for. It does fix the problem, and
it seems to be a better solution.
Arnd
I remember seeing this patch, but I don't remember the exact context.
But when you think about it, !!cond can be a build time constant even
if cond is not, as long as you can prove statically that cond != 0. So
You're right. I just tested it and to my surprise gcc is smart enough
to figure that case out.
quoted
I think this change is obviously correct, and an improvement since it
will remove the profiling overhead of branches that are not true
branches in the first place.
Indeed.
... and perhaps we should not evaluate cond twice either?
It is not. The value of the argument to __builtin_constant_p() is not
itself evaluated and therefore does not produce side effects.