[U-Boot] Pull request: u-boot-mips

[Daniel Schwierzeck]

Hi Paul,

2017-07-19 15:04 GMT+02:00 Tom Rini <trini at 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 at 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,
>> > >
>> > >     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!
>>
>> 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.

-- 
- Daniel