[U-Boot] Pull request: u-boot-mips
Hi Tom,
This supports dynamic relocation on MIPS without the need for building a position-independent executable. This notably reduces the code size for all MIPS boards.
The following changes since commit d85ca029f257b53a96da6c2fb421e78a003a9943:
Prepare v2017.07 (2017-07-10 13:07:38 -0400)
are available in the git repository at:
git://git.denx.de/u-boot-mips.git master
for you to fetch changes up to f653dcd5720c4135607211f7304283d7a8ec3b8a:
MIPS: bootm: Fix broken boot_env_legacy codepath (2017-07-12 22:10:42 +0200)
---------------------------------------------------------------- Paul Burton (2): Makefile: Allow arch post-link hook MIPS: Stop building position independent code
Zubair Lutfullah Kakakhel (1): MIPS: bootm: Fix broken boot_env_legacy codepath
Makefile | 7 +- arch/mips/Makefile.postlink | 23 ++++++ arch/mips/config.mk | 21 ++--- arch/mips/cpu/start.S | 130 ------------------------------- arch/mips/cpu/u-boot.lds | 41 +++------- arch/mips/include/asm/relocs.h | 24 ++++++ arch/mips/include/asm/sections.h | 7 ++ arch/mips/lib/Makefile | 1 + arch/mips/lib/bootm.c | 8 +- arch/mips/lib/reloc.c | 164 +++++++++++++++++++++++++++++++++++++++ common/board_f.c | 2 +- tools/.gitignore | 1 + tools/Makefile | 2 + tools/mips-relocs.c | 426 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 14 files changed, 672 insertions(+), 185 deletions(-) create mode 100644 arch/mips/Makefile.postlink create mode 100644 arch/mips/include/asm/relocs.h create mode 100644 arch/mips/lib/reloc.c create mode 100644 tools/mips-relocs.c
On Wed, Jul 12, 2017 at 10:32:29PM +0200, Daniel Schwierzeck wrote:
Hi Tom,
This supports dynamic relocation on MIPS without the need for building a position-independent executable. This notably reduces the code size for all MIPS boards.
The following changes since commit d85ca029f257b53a96da6c2fb421e78a003a9943:
Prepare v2017.07 (2017-07-10 13:07:38 -0400)
are available in the git repository at:
git://git.denx.de/u-boot-mips.git master
for you to fetch changes up to f653dcd5720c4135607211f7304283d7a8ec3b8a:
MIPS: bootm: Fix broken boot_env_legacy codepath (2017-07-12 22:10:42 +0200)
I'm seeing: mips: + tplink_wdr4300 +(tplink_wdr4300) pfx##hdr32[idx].field = _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:51:11: note: ?_val? was declared here +(tplink_wdr4300) uint64_t _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:88:2: note: in expansion of macro ?set_hdr_field? +(tplink_wdr4300) set_hdr_field(p, idx, field, val) +(tplink_wdr4300) ^~~~~~~~~~~~~ +(tplink_wdr4300) ../tools/mips-relocs.c:408:3: note: in expansion of macro ?set_phdr_field? +(tplink_wdr4300) set_phdr_field(i, p_filesz, load_sz); +(tplink_wdr4300) ^~~~~~~~~~~~~~ w+(tplink_wdr4300) ../tools/mips-relocs.c: In function ?main?: w+(tplink_wdr4300) ../tools/mips-relocs.c:77:25: warning: ?_val? may be used uninitialized in this function [-Wmaybe-uninitialized]
for what I suspect is going to be all MIPS. Host tools here are gcc-6.3.
On Wed, Jul 12, 2017 at 04:57:42PM -0400, Tom Rini wrote:
On Wed, Jul 12, 2017 at 10:32:29PM +0200, Daniel Schwierzeck wrote:
Hi Tom,
This supports dynamic relocation on MIPS without the need for building a position-independent executable. This notably reduces the code size for all MIPS boards.
The following changes since commit d85ca029f257b53a96da6c2fb421e78a003a9943:
Prepare v2017.07 (2017-07-10 13:07:38 -0400)
are available in the git repository at:
git://git.denx.de/u-boot-mips.git master
for you to fetch changes up to f653dcd5720c4135607211f7304283d7a8ec3b8a:
MIPS: bootm: Fix broken boot_env_legacy codepath (2017-07-12 22:10:42 +0200)
I'm seeing: mips: + tplink_wdr4300 +(tplink_wdr4300) pfx##hdr32[idx].field = _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:51:11: note: ?_val? was declared here +(tplink_wdr4300) uint64_t _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:88:2: note: in expansion of macro ?set_hdr_field? +(tplink_wdr4300) set_hdr_field(p, idx, field, val) +(tplink_wdr4300) ^~~~~~~~~~~~~ +(tplink_wdr4300) ../tools/mips-relocs.c:408:3: note: in expansion of macro ?set_phdr_field? +(tplink_wdr4300) set_phdr_field(i, p_filesz, load_sz); +(tplink_wdr4300) ^~~~~~~~~~~~~~ w+(tplink_wdr4300) ../tools/mips-relocs.c: In function ?main?: w+(tplink_wdr4300) ../tools/mips-relocs.c:77:25: warning: ?_val? may be used uninitialized in this function [-Wmaybe-uninitialized]
for what I suspect is going to be all MIPS. Host tools here are gcc-6.3.
Yeah, this is all MIPS boards. Please fix, thanks!
Hi Paul,
2017-07-13 2:33 GMT+02:00 Tom Rini trini@konsulko.com:
On Wed, Jul 12, 2017 at 04:57:42PM -0400, Tom Rini wrote:
On Wed, Jul 12, 2017 at 10:32:29PM +0200, Daniel Schwierzeck wrote:
Hi Tom,
This supports dynamic relocation on MIPS without the need for building a position-independent executable. This notably reduces the code size for all MIPS boards.
The following changes since commit d85ca029f257b53a96da6c2fb421e78a003a9943:
Prepare v2017.07 (2017-07-10 13:07:38 -0400)
are available in the git repository at:
git://git.denx.de/u-boot-mips.git master
for you to fetch changes up to f653dcd5720c4135607211f7304283d7a8ec3b8a:
MIPS: bootm: Fix broken boot_env_legacy codepath (2017-07-12 22:10:42 +0200)
I'm seeing: mips: + tplink_wdr4300 +(tplink_wdr4300) pfx##hdr32[idx].field = _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:51:11: note: ?_val? was declared here +(tplink_wdr4300) uint64_t _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:88:2: note: in expansion of macro ?set_hdr_field? +(tplink_wdr4300) set_hdr_field(p, idx, field, val) +(tplink_wdr4300) ^~~~~~~~~~~~~ +(tplink_wdr4300) ../tools/mips-relocs.c:408:3: note: in expansion of macro ?set_phdr_field? +(tplink_wdr4300) set_phdr_field(i, p_filesz, load_sz); +(tplink_wdr4300) ^~~~~~~~~~~~~~ w+(tplink_wdr4300) ../tools/mips-relocs.c: In function ?main?: w+(tplink_wdr4300) ../tools/mips-relocs.c:77:25: warning: ?_val? may be used uninitialized in this function [-Wmaybe-uninitialized]
for what I suspect is going to be all MIPS. Host tools here are gcc-6.3.
Yeah, this is all MIPS boards. Please fix, thanks!
Paul, could you send a follow-up patch to fix this? Thanks.
Hi Daniel & Tom,
On Thursday, 13 July 2017 03:51:00 PDT Daniel Schwierzeck wrote:
Hi Paul,
2017-07-13 2:33 GMT+02:00 Tom Rini trini@konsulko.com:
On Wed, Jul 12, 2017 at 04:57:42PM -0400, Tom Rini wrote:
On Wed, Jul 12, 2017 at 10:32:29PM +0200, Daniel Schwierzeck wrote:
Hi Tom,
This supports dynamic relocation on MIPS without the need for building a position-independent executable. This notably reduces the code size for all MIPS boards.
The following changes since commit
d85ca029f257b53a96da6c2fb421e78a003a9943:
Prepare v2017.07 (2017-07-10 13:07:38 -0400)
are available in the git repository at: git://git.denx.de/u-boot-mips.git master
for you to fetch changes up to
f653dcd5720c4135607211f7304283d7a8ec3b8a:
MIPS: bootm: Fix broken boot_env_legacy codepath (2017-07-12 22:10:42 +0200)>>
I'm seeing: mips: + tplink_wdr4300
+(tplink_wdr4300) pfx##hdr32[idx].field = _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:51:11: note: ?_val? was declared here +(tplink_wdr4300) uint64_t _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:88:2: note: in expansion of macro ?set_hdr_field? +(tplink_wdr4300) set_hdr_field(p, idx, field, val) +(tplink_wdr4300) ^~~~~~~~~~~~~ +(tplink_wdr4300) ../tools/mips-relocs.c:408:3: note: in expansion of macro ?set_phdr_field? +(tplink_wdr4300) set_phdr_field(i, p_filesz, load_sz); +(tplink_wdr4300) ^~~~~~~~~~~~~~ w+(tplink_wdr4300) ../tools/mips-relocs.c: In function ?main?: w+(tplink_wdr4300) ../tools/mips-relocs.c:77:25: warning: ?_val? may be used uninitialized in this function [-Wmaybe-uninitialized]
for what I suspect is going to be all MIPS. Host tools here are gcc-6.3.
Yeah, this is all MIPS boards. Please fix, thanks!
Paul, could you send a follow-up patch to fix this? Thanks.
Sure. I'm on gcc 7.1.1 which doesn't show this issue. Is the following sufficient to fix this for you Tom? I can submit it as a proper patch if you like & it works out.
Thanks, Paul
diff --git a/tools/mips-relocs.c b/tools/mips-relocs.c index b690fa53c4..75d532546b 100644 --- a/tools/mips-relocs.c +++ b/tools/mips-relocs.c @@ -69,6 +69,9 @@ case 8: \ _val = is_be ? htobe64(val) : htole64(val); \ break; \ + default: \ + __builtin_unreachable(); \ + break; \ } \ \ if (is_64) \
On Thu, Jul 13, 2017 at 10:58:47AM -0700, Paul Burton wrote:
Hi Daniel & Tom,
On Thursday, 13 July 2017 03:51:00 PDT Daniel Schwierzeck wrote:
Hi Paul,
2017-07-13 2:33 GMT+02:00 Tom Rini trini@konsulko.com:
On Wed, Jul 12, 2017 at 04:57:42PM -0400, Tom Rini wrote:
On Wed, Jul 12, 2017 at 10:32:29PM +0200, Daniel Schwierzeck wrote:
Hi Tom,
This supports dynamic relocation on MIPS without the need for building a position-independent executable. This notably reduces the code size for all MIPS boards.
The following changes since commit
d85ca029f257b53a96da6c2fb421e78a003a9943:
Prepare v2017.07 (2017-07-10 13:07:38 -0400)
are available in the git repository at: git://git.denx.de/u-boot-mips.git master
for you to fetch changes up to
f653dcd5720c4135607211f7304283d7a8ec3b8a:
MIPS: bootm: Fix broken boot_env_legacy codepath (2017-07-12 22:10:42 +0200)>>
I'm seeing: mips: + tplink_wdr4300
+(tplink_wdr4300) pfx##hdr32[idx].field = _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:51:11: note: ?_val? was declared here +(tplink_wdr4300) uint64_t _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:88:2: note: in expansion of macro ?set_hdr_field? +(tplink_wdr4300) set_hdr_field(p, idx, field, val) +(tplink_wdr4300) ^~~~~~~~~~~~~ +(tplink_wdr4300) ../tools/mips-relocs.c:408:3: note: in expansion of macro ?set_phdr_field? +(tplink_wdr4300) set_phdr_field(i, p_filesz, load_sz); +(tplink_wdr4300) ^~~~~~~~~~~~~~ w+(tplink_wdr4300) ../tools/mips-relocs.c: In function ?main?: w+(tplink_wdr4300) ../tools/mips-relocs.c:77:25: warning: ?_val? may be used uninitialized in this function [-Wmaybe-uninitialized]
for what I suspect is going to be all MIPS. Host tools here are gcc-6.3.
Yeah, this is all MIPS boards. Please fix, thanks!
Paul, could you send a follow-up patch to fix this? Thanks.
Sure. I'm on gcc 7.1.1 which doesn't show this issue. Is the following sufficient to fix this for you Tom? I can submit it as a proper patch if you like & it works out.
Oh? That it doesn't show up with a newer compiler is interesting...
Thanks, Paul
diff --git a/tools/mips-relocs.c b/tools/mips-relocs.c index b690fa53c4..75d532546b 100644 --- a/tools/mips-relocs.c +++ b/tools/mips-relocs.c @@ -69,6 +69,9 @@ case 8: \ _val = is_be ? htobe64(val) : htole64(val); \ break; \
default: \__builtin_unreachable(); \break; \ } \
I'm not a huge fan of adding builtin calls like this. Is there some other way to restructure the code perhaps, while still being clear? Thanks!
Hi Tom,
On Tuesday, 18 July 2017 02:07:59 BST Tom Rini wrote:
On Thu, Jul 13, 2017 at 10:58:47AM -0700, Paul Burton wrote:
Hi Daniel & Tom,
On Thursday, 13 July 2017 03:51:00 PDT Daniel Schwierzeck wrote:
Hi Paul,
2017-07-13 2:33 GMT+02:00 Tom Rini trini@konsulko.com:
On Wed, Jul 12, 2017 at 04:57:42PM -0400, Tom Rini wrote:
On Wed, Jul 12, 2017 at 10:32:29PM +0200, Daniel Schwierzeck wrote:
Hi Tom,
This supports dynamic relocation on MIPS without the need for building a position-independent executable. This notably reduces the code size for all MIPS boards.
The following changes since commit
d85ca029f257b53a96da6c2fb421e78a003a9943:
Prepare v2017.07 (2017-07-10 13:07:38 -0400)
are available in the git repository at: git://git.denx.de/u-boot-mips.git master
for you to fetch changes up to
f653dcd5720c4135607211f7304283d7a8ec3b8a:
MIPS: bootm: Fix broken boot_env_legacy codepath (2017-07-12 22:10:42 +0200)>>
I'm seeing: mips: + tplink_wdr4300
+(tplink_wdr4300) pfx##hdr32[idx].field = _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:51:11: note: ?_val? was declared here +(tplink_wdr4300) uint64_t _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:88:2: note: in expansion of macro ?set_hdr_field? +(tplink_wdr4300) set_hdr_field(p, idx, field, val) +(tplink_wdr4300) ^~~~~~~~~~~~~ +(tplink_wdr4300) ../tools/mips-relocs.c:408:3: note: in expansion of macro ?set_phdr_field? +(tplink_wdr4300) set_phdr_field(i, p_filesz, load_sz); +(tplink_wdr4300) ^~~~~~~~~~~~~~ w+(tplink_wdr4300) ../tools/mips-relocs.c: In function ?main?: w+(tplink_wdr4300) ../tools/mips-relocs.c:77:25: warning: ?_val? may be used uninitialized in this function [-Wmaybe-uninitialized]
for what I suspect is going to be all MIPS. Host tools here are gcc-6.3.
Yeah, this is all MIPS boards. Please fix, thanks!
Paul, could you send a follow-up patch to fix this? Thanks.
Sure. I'm on gcc 7.1.1 which doesn't show this issue. Is the following sufficient to fix this for you Tom? I can submit it as a proper patch if you like & it works out.
Oh? That it doesn't show up with a newer compiler is interesting...
Yeah, I imagine gcc got smarter at recognising that the path it was complaining about is never actually taken.
Thanks,
Pauldiff --git a/tools/mips-relocs.c b/tools/mips-relocs.c index b690fa53c4..75d532546b 100644 --- a/tools/mips-relocs.c +++ b/tools/mips-relocs.c @@ -69,6 +69,9 @@
case 8: \ _val = is_be ? htobe64(val) : htole64(val); \ break; \
default: \__builtin_unreachable(); \break; \ } \I'm not a huge fan of adding builtin calls like this. Is there some other way to restructure the code perhaps, while still being clear? Thanks!
An alternative would be to assign _val = 0 to silence the warning, and probably call abort() or assert(0) or something similar in that path. Would that be preferrable to you?
Thanks, Paul
On Wed, Jul 19, 2017 at 01:59:16PM +0100, Paul Burton wrote:
Hi Tom,
On Tuesday, 18 July 2017 02:07:59 BST Tom Rini wrote:
On Thu, Jul 13, 2017 at 10:58:47AM -0700, Paul Burton wrote:
Hi Daniel & Tom,
On Thursday, 13 July 2017 03:51:00 PDT Daniel Schwierzeck wrote:
Hi Paul,
2017-07-13 2:33 GMT+02:00 Tom Rini trini@konsulko.com:
On Wed, Jul 12, 2017 at 04:57:42PM -0400, Tom Rini wrote:
On Wed, Jul 12, 2017 at 10:32:29PM +0200, Daniel Schwierzeck wrote: > Hi Tom, > > This supports dynamic relocation on MIPS without the need for > building > a > position-independent executable. This notably reduces the code size > for > all MIPS boards. > > The following changes since commit
d85ca029f257b53a96da6c2fb421e78a003a9943:
> Prepare v2017.07 (2017-07-10 13:07:38 -0400) > > are available in the git repository at: > git://git.denx.de/u-boot-mips.git master > > for you to fetch changes up to
f653dcd5720c4135607211f7304283d7a8ec3b8a:
> MIPS: bootm: Fix broken boot_env_legacy codepath (2017-07-12 > 22:10:42 > +0200)>>
I'm seeing: mips: + tplink_wdr4300
+(tplink_wdr4300) pfx##hdr32[idx].field = _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:51:11: note: ?_val? was declared here +(tplink_wdr4300) uint64_t _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:88:2: note: in expansion of macro ?set_hdr_field? +(tplink_wdr4300) set_hdr_field(p, idx, field, val) +(tplink_wdr4300) ^~~~~~~~~~~~~ +(tplink_wdr4300) ../tools/mips-relocs.c:408:3: note: in expansion of macro ?set_phdr_field? +(tplink_wdr4300) set_phdr_field(i, p_filesz, load_sz); +(tplink_wdr4300) ^~~~~~~~~~~~~~ w+(tplink_wdr4300) ../tools/mips-relocs.c: In function ?main?: w+(tplink_wdr4300) ../tools/mips-relocs.c:77:25: warning: ?_val? may be used uninitialized in this function [-Wmaybe-uninitialized]
for what I suspect is going to be all MIPS. Host tools here are gcc-6.3.
Yeah, this is all MIPS boards. Please fix, thanks!
Paul, could you send a follow-up patch to fix this? Thanks.
Sure. I'm on gcc 7.1.1 which doesn't show this issue. Is the following sufficient to fix this for you Tom? I can submit it as a proper patch if you like & it works out.
Oh? That it doesn't show up with a newer compiler is interesting...
Yeah, I imagine gcc got smarter at recognising that the path it was complaining about is never actually taken.
Thanks,
Pauldiff --git a/tools/mips-relocs.c b/tools/mips-relocs.c index b690fa53c4..75d532546b 100644 --- a/tools/mips-relocs.c +++ b/tools/mips-relocs.c @@ -69,6 +69,9 @@
case 8: \ _val = is_be ? htobe64(val) : htole64(val); \ break; \
default: \__builtin_unreachable(); \break; \ } \I'm not a huge fan of adding builtin calls like this. Is there some other way to restructure the code perhaps, while still being clear? Thanks!
An alternative would be to assign _val = 0 to silence the warning, and probably call abort() or assert(0) or something similar in that path. Would that be preferrable to you?
Yeah, thanks!
Hi Paul,
2017-07-19 15:04 GMT+02:00 Tom Rini trini@konsulko.com:
On Wed, Jul 19, 2017 at 01:59:16PM +0100, Paul Burton wrote:
Hi Tom,
On Tuesday, 18 July 2017 02:07:59 BST Tom Rini wrote:
On Thu, Jul 13, 2017 at 10:58:47AM -0700, Paul Burton wrote:
Hi Daniel & Tom,
On Thursday, 13 July 2017 03:51:00 PDT Daniel Schwierzeck wrote:
Hi Paul,
2017-07-13 2:33 GMT+02:00 Tom Rini trini@konsulko.com:
On Wed, Jul 12, 2017 at 04:57:42PM -0400, Tom Rini wrote: > On Wed, Jul 12, 2017 at 10:32:29PM +0200, Daniel Schwierzeck wrote: > > Hi Tom, > > > > This supports dynamic relocation on MIPS without the need for > > building > > a > > position-independent executable. This notably reduces the code size > > for > > all MIPS boards. > > > > The following changes since commit
d85ca029f257b53a96da6c2fb421e78a003a9943:
> > Prepare v2017.07 (2017-07-10 13:07:38 -0400) > > > > are available in the git repository at: > > git://git.denx.de/u-boot-mips.git master > > > > for you to fetch changes up to
f653dcd5720c4135607211f7304283d7a8ec3b8a:
> > MIPS: bootm: Fix broken boot_env_legacy codepath (2017-07-12 > > 22:10:42 > > +0200)>> > > I'm seeing: > mips: + tplink_wdr4300 > > +(tplink_wdr4300) pfx##hdr32[idx].field = _val; \ > +(tplink_wdr4300) ^ > +(tplink_wdr4300) ../tools/mips-relocs.c:51:11: note: ?_val? was > declared > here +(tplink_wdr4300) uint64_t _val; \ > +(tplink_wdr4300) ^ > +(tplink_wdr4300) ../tools/mips-relocs.c:88:2: note: in expansion of > macro ?set_hdr_field? +(tplink_wdr4300) set_hdr_field(p, idx, > field, > val) > +(tplink_wdr4300) ^~~~~~~~~~~~~ > +(tplink_wdr4300) ../tools/mips-relocs.c:408:3: note: in expansion of > macro ?set_phdr_field? +(tplink_wdr4300) set_phdr_field(i, > p_filesz, > load_sz); > +(tplink_wdr4300) ^~~~~~~~~~~~~~ > w+(tplink_wdr4300) ../tools/mips-relocs.c: In function ?main?: > w+(tplink_wdr4300) ../tools/mips-relocs.c:77:25: warning: ?_val? may > be > used uninitialized in this function [-Wmaybe-uninitialized] > > for what I suspect is going to be all MIPS. Host tools here are > gcc-6.3.
Yeah, this is all MIPS boards. Please fix, thanks!
Paul, could you send a follow-up patch to fix this? Thanks.
Sure. I'm on gcc 7.1.1 which doesn't show this issue. Is the following sufficient to fix this for you Tom? I can submit it as a proper patch if you like & it works out.
Oh? That it doesn't show up with a newer compiler is interesting...
Yeah, I imagine gcc got smarter at recognising that the path it was complaining about is never actually taken.
Thanks,
Pauldiff --git a/tools/mips-relocs.c b/tools/mips-relocs.c index b690fa53c4..75d532546b 100644 --- a/tools/mips-relocs.c +++ b/tools/mips-relocs.c @@ -69,6 +69,9 @@
case 8: \ _val = is_be ? htobe64(val) : htole64(val); \ break; \
default: \__builtin_unreachable(); \break; \ } \I'm not a huge fan of adding builtin calls like this. Is there some other way to restructure the code perhaps, while still being clear? Thanks!
An alternative would be to assign _val = 0 to silence the warning, and probably call abort() or assert(0) or something similar in that path. Would that be preferrable to you?
Yeah, thanks!
do you have a patch ready? I'd like to resend the pull request before -rc1, thanks.
It seems that gcc 6.3 at least is smart enough to warn about the _val variable being unassigned in the default case in the set_hdr_field() macro, but not smart enough to figure out that the default case is never taken. This results in warnings such as the following:
pfx##hdr32[idx].field = _val; \ ^ ../tools/mips-relocs.c:51:11: note: _val was declared here uint64_t _val; \ ^ ../tools/mips-relocs.c:88:2: note: in expansion of macro set_hdr_field set_hdr_field(p, idx, field, val) ^~~~~~~~~~~~~ ../tools/mips-relocs.c:408:3: note: in expansion of macro set_phdr_field set_phdr_field(i, p_filesz, load_sz); ^~~~~~~~~~~~~~ ../tools/mips-relocs.c: In function main: ../tools/mips-relocs.c:77:25: warning: _val may be used uninitialized in this function [-Wmaybe-uninitialized]
Avoid this by assigning _val = 0 in the default case, and asserting that we didn't actually hit it for good measure.
For reference gcc 7.1.1 seems to be smart enough to not hit the above warning without this patch.
Signed-off-by: Paul Burton paul.burton@imgtec.com Fixes: 011dd93ca97a ("MIPS: Stop building position independent code") Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Tom Rini trini@konsulko.com Cc: u-boot@lists.denx.de --- Feel free to fold this in as a fixup for 011dd93ca97a ("MIPS: Stop building position independent code") if preferred. --- tools/mips-relocs.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/tools/mips-relocs.c b/tools/mips-relocs.c index b690fa53c4..8be69d320f 100644 --- a/tools/mips-relocs.c +++ b/tools/mips-relocs.c @@ -6,6 +6,7 @@ * SPDX-License-Identifier: GPL-2.0+ */
+#include <assert.h> #include <elf.h> #include <errno.h> #include <fcntl.h> @@ -69,6 +70,11 @@ case 8: \ _val = is_be ? htobe64(val) : htole64(val); \ break; \ + default: \ + /* We should never reach here */ \ + _val = 0; \ + assert(0); \ + break; \ } \ \ if (is_64) \
Hi Daniel,
On Tuesday, 25 July 2017 14:38:44 BST Daniel Schwierzeck wrote:
Hi Paul,
2017-07-19 15:04 GMT+02:00 Tom Rini trini@konsulko.com:
On Wed, Jul 19, 2017 at 01:59:16PM +0100, Paul Burton wrote:
Hi Tom,
On Tuesday, 18 July 2017 02:07:59 BST Tom Rini wrote:
On Thu, Jul 13, 2017 at 10:58:47AM -0700, Paul Burton wrote:
Hi Daniel & Tom,
On Thursday, 13 July 2017 03:51:00 PDT Daniel Schwierzeck wrote:
Hi Paul,
2017-07-13 2:33 GMT+02:00 Tom Rini trini@konsulko.com: > On Wed, Jul 12, 2017 at 04:57:42PM -0400, Tom Rini wrote: >> On Wed, Jul 12, 2017 at 10:32:29PM +0200, Daniel Schwierzeck
wrote:
>> > Hi Tom, >> > >> > This supports dynamic relocation on MIPS without the need for >> > building >> > a >> > position-independent executable. This notably reduces the code >> > size >> > for >> > all MIPS boards. >> > >> > The following changes since commit
d85ca029f257b53a96da6c2fb421e78a003a9943:
>> > Prepare v2017.07 (2017-07-10 13:07:38 -0400) >> > >> > are available in the git repository at: >> > git://git.denx.de/u-boot-mips.git master >> > >> > for you to fetch changes up to
f653dcd5720c4135607211f7304283d7a8ec3b8a:
>> > MIPS: bootm: Fix broken boot_env_legacy codepath (2017-07-12 >> > 22:10:42 >> > +0200)>> >> >> I'm seeing: >> mips: + tplink_wdr4300 >> >> +(tplink_wdr4300) pfx##hdr32[idx].field = _val; \ >> +(tplink_wdr4300) ^ >> +(tplink_wdr4300) ../tools/mips-relocs.c:51:11: note: ?_val? was >> declared >> here +(tplink_wdr4300) uint64_t _val; \ >> +(tplink_wdr4300) ^ >> +(tplink_wdr4300) ../tools/mips-relocs.c:88:2: note: in >> expansion of >> macro ?set_hdr_field? +(tplink_wdr4300) set_hdr_field(p, idx, >> field, >> val) >> +(tplink_wdr4300) ^~~~~~~~~~~~~ >> +(tplink_wdr4300) ../tools/mips-relocs.c:408:3: note: in >> expansion of >> macro ?set_phdr_field? +(tplink_wdr4300) set_phdr_field(i, >> p_filesz, >> load_sz); >> +(tplink_wdr4300) ^~~~~~~~~~~~~~ >> w+(tplink_wdr4300) ../tools/mips-relocs.c: In function ?main?: >> w+(tplink_wdr4300) ../tools/mips-relocs.c:77:25: warning: ?_val? >> may >> be >> used uninitialized in this function [-Wmaybe-uninitialized] >> >> for what I suspect is going to be all MIPS. Host tools here are >> gcc-6.3. > > Yeah, this is all MIPS boards. Please fix, thanks!
Paul, could you send a follow-up patch to fix this? Thanks.
Sure. I'm on gcc 7.1.1 which doesn't show this issue. Is the following sufficient to fix this for you Tom? I can submit it as a proper patch if you like & it works out.
Oh? That it doesn't show up with a newer compiler is interesting...
Yeah, I imagine gcc got smarter at recognising that the path it was complaining about is never actually taken.
Thanks,
Pauldiff --git a/tools/mips-relocs.c b/tools/mips-relocs.c index b690fa53c4..75d532546b 100644 --- a/tools/mips-relocs.c +++ b/tools/mips-relocs.c @@ -69,6 +69,9 @@
case 8: \ _val = is_be ? htobe64(val) : htole64(val); \ break; \
default: \__builtin_unreachable(); \break; \ } \I'm not a huge fan of adding builtin calls like this. Is there some other way to restructure the code perhaps, while still being clear? Thanks!
An alternative would be to assign _val = 0 to silence the warning, and probably call abort() or assert(0) or something similar in that path. Would that be preferrable to you?
Yeah, thanks!
do you have a patch ready? I'd like to resend the pull request before -rc1, thanks.
Apologies - I'm currently travelling & this had slipped down my list. I've just submitted "mips-relocs: Fix warning from gcc 6.3" which should hopefully resolve this.
Thanks, Paul
It seems that gcc 6.3 at least is smart enough to warn about the _val variable being unassigned in the default case in the set_hdr_field() macro, but not smart enough to figure out that the default case is never taken. This results in warnings such as the following:
pfx##hdr32[idx].field = _val; \ ^ ../tools/mips-relocs.c:51:11: note: _val was declared here uint64_t _val; \ ^ ../tools/mips-relocs.c:88:2: note: in expansion of macro set_hdr_field set_hdr_field(p, idx, field, val) ^~~~~~~~~~~~~ ../tools/mips-relocs.c:408:3: note: in expansion of macro set_phdr_field set_phdr_field(i, p_filesz, load_sz); ^~~~~~~~~~~~~~ ../tools/mips-relocs.c: In function main: ../tools/mips-relocs.c:77:25: warning: _val may be used uninitialized in this function [-Wmaybe-uninitialized]
Avoid this by assigning _val = 0 in the default case, and asserting that we didn't actually hit it for good measure.
For reference gcc 7.1.1 seems to be smart enough to not hit the above warning without this patch.
Signed-off-by: Paul Burton paul.burton@imgtec.com Fixes: 011dd93ca97a ("MIPS: Stop building position independent code") Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Tom Rini trini@konsulko.com Cc: u-boot@lists.denx.de --- Feel free to fold this in as a fixup for 011dd93ca97a ("MIPS: Stop building position independent code") if preferred. --- tools/mips-relocs.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/tools/mips-relocs.c b/tools/mips-relocs.c index b690fa53c4..8be69d320f 100644 --- a/tools/mips-relocs.c +++ b/tools/mips-relocs.c @@ -6,6 +6,7 @@ * SPDX-License-Identifier: GPL-2.0+ */
+#include <assert.h> #include <elf.h> #include <errno.h> #include <fcntl.h> @@ -69,6 +70,11 @@ case 8: \ _val = is_be ? htobe64(val) : htole64(val); \ break; \ + default: \ + /* We should never reach here */ \ + _val = 0; \ + assert(0); \ + break; \ } \ \ if (is_64) \
Hi Tom,
2017-07-12 22:57 GMT+02:00 Tom Rini trini@konsulko.com:
On Wed, Jul 12, 2017 at 10:32:29PM +0200, Daniel Schwierzeck wrote:
Hi Tom,
This supports dynamic relocation on MIPS without the need for building a position-independent executable. This notably reduces the code size for all MIPS boards.
The following changes since commit d85ca029f257b53a96da6c2fb421e78a003a9943:
Prepare v2017.07 (2017-07-10 13:07:38 -0400)
are available in the git repository at:
git://git.denx.de/u-boot-mips.git master
for you to fetch changes up to f653dcd5720c4135607211f7304283d7a8ec3b8a:
MIPS: bootm: Fix broken boot_env_legacy codepath (2017-07-12 22:10:42 +0200)
I'm seeing: mips: + tplink_wdr4300 +(tplink_wdr4300) pfx##hdr32[idx].field = _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:51:11: note: ?_val? was declared here +(tplink_wdr4300) uint64_t _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:88:2: note: in expansion of macro ?set_hdr_field? +(tplink_wdr4300) set_hdr_field(p, idx, field, val) +(tplink_wdr4300) ^~~~~~~~~~~~~ +(tplink_wdr4300) ../tools/mips-relocs.c:408:3: note: in expansion of macro ?set_phdr_field? +(tplink_wdr4300) set_phdr_field(i, p_filesz, load_sz); +(tplink_wdr4300) ^~~~~~~~~~~~~~ w+(tplink_wdr4300) ../tools/mips-relocs.c: In function ?main?: w+(tplink_wdr4300) ../tools/mips-relocs.c:77:25: warning: ?_val? may be used uninitialized in this function [-Wmaybe-uninitialized]
for what I suspect is going to be all MIPS. Host tools here are gcc-6.3.
how about adding a separate build step in Travis CI for all host tools and building that step with different compilers (recent gcc or clang)? Or we always install a recent x86_64 toolchain and override the host toolchain contained in the Ubuntu builder image. This toolchain is then used for all build steps.
On Thu, Jul 13, 2017 at 01:05:33PM +0200, Daniel Schwierzeck wrote:
Hi Tom,
2017-07-12 22:57 GMT+02:00 Tom Rini trini@konsulko.com:
On Wed, Jul 12, 2017 at 10:32:29PM +0200, Daniel Schwierzeck wrote:
Hi Tom,
This supports dynamic relocation on MIPS without the need for building a position-independent executable. This notably reduces the code size for all MIPS boards.
The following changes since commit d85ca029f257b53a96da6c2fb421e78a003a9943:
Prepare v2017.07 (2017-07-10 13:07:38 -0400)
are available in the git repository at:
git://git.denx.de/u-boot-mips.git master
for you to fetch changes up to f653dcd5720c4135607211f7304283d7a8ec3b8a:
MIPS: bootm: Fix broken boot_env_legacy codepath (2017-07-12 22:10:42 +0200)
I'm seeing: mips: + tplink_wdr4300 +(tplink_wdr4300) pfx##hdr32[idx].field = _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:51:11: note: ?_val? was declared here +(tplink_wdr4300) uint64_t _val; \ +(tplink_wdr4300) ^ +(tplink_wdr4300) ../tools/mips-relocs.c:88:2: note: in expansion of macro ?set_hdr_field? +(tplink_wdr4300) set_hdr_field(p, idx, field, val) +(tplink_wdr4300) ^~~~~~~~~~~~~ +(tplink_wdr4300) ../tools/mips-relocs.c:408:3: note: in expansion of macro ?set_phdr_field? +(tplink_wdr4300) set_phdr_field(i, p_filesz, load_sz); +(tplink_wdr4300) ^~~~~~~~~~~~~~ w+(tplink_wdr4300) ../tools/mips-relocs.c: In function ?main?: w+(tplink_wdr4300) ../tools/mips-relocs.c:77:25: warning: ?_val? may be used uninitialized in this function [-Wmaybe-uninitialized]
for what I suspect is going to be all MIPS. Host tools here are gcc-6.3.
how about adding a separate build step in Travis CI for all host tools and building that step with different compilers (recent gcc or clang)? Or we always install a recent x86_64 toolchain and override the host toolchain contained in the Ubuntu builder image. This toolchain is then used for all build steps.
Travis CI doesn't fail on warning only. I'd like to move us to an optional -Werror, but we're not there yet either. That said, we might be able to force clang to be used, which would probably have caught that, even in the version shipped in Ubuntu 14.04, more easily than we can update the host tools on the VMs. But I'd be happy to look at patches and I know you can tell travis to add feeds and then update things.
Hi Tom,
second try with the gcc 6.3 build fix applied and rebased to current master.
The following changes since commit d56b4b19744c314c26dc77585a7c7a9253d1487d:
configs: Migrate RBTREE, LZO, CMD_MTDPARTS, CMD_UBI and CMD_UBIFS (2017-07-24 20:35:55 -0400)
are available in the git repository at:
git://git.denx.de/u-boot-mips.git master
for you to fetch changes up to e94136bd87b18345e38a5e44445a476de12b4354:
mips-relocs: Fix warning from gcc 6.3 (2017-07-25 20:44:38 +0200)
---------------------------------------------------------------- Paul Burton (3): Makefile: Allow arch post-link hook MIPS: Stop building position independent code mips-relocs: Fix warning from gcc 6.3
Zubair Lutfullah Kakakhel (1): MIPS: bootm: Fix broken boot_env_legacy codepath
Makefile | 7 +- arch/mips/Makefile.postlink | 23 +++++++ arch/mips/config.mk | 21 ++---- arch/mips/cpu/start.S | 130 ------------------------------------ arch/mips/cpu/u-boot.lds | 41 +++--------- arch/mips/include/asm/relocs.h | 24 +++++++ arch/mips/include/asm/sections.h | 7 ++ arch/mips/lib/Makefile | 1 + arch/mips/lib/bootm.c | 8 +-- arch/mips/lib/reloc.c | 164 ++++++++++++++++++++++++++++++++++++++++++++++ common/board_f.c | 2 +- tools/.gitignore | 1 + tools/Makefile | 2 + tools/mips-relocs.c | 432 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 14 files changed, 678 insertions(+), 185 deletions(-) create mode 100644 arch/mips/Makefile.postlink create mode 100644 arch/mips/include/asm/relocs.h create mode 100644 arch/mips/lib/reloc.c create mode 100644 tools/mips-relocs.c
On Tue, Jul 25, 2017 at 08:55:23PM +0200, Daniel Schwierzeck wrote:
Hi Tom,
second try with the gcc 6.3 build fix applied and rebased to current master.
The following changes since commit d56b4b19744c314c26dc77585a7c7a9253d1487d:
configs: Migrate RBTREE, LZO, CMD_MTDPARTS, CMD_UBI and CMD_UBIFS (2017-07-24 20:35:55 -0400)
are available in the git repository at:
git://git.denx.de/u-boot-mips.git master
for you to fetch changes up to e94136bd87b18345e38a5e44445a476de12b4354:
mips-relocs: Fix warning from gcc 6.3 (2017-07-25 20:44:38 +0200)
Applied to u-boot/master, thanks!
participants (3)
-
Daniel Schwierzeck -
Paul Burton -
Tom Rini