Patchwork [U-Boot] U-Boot Bug with newer GCC

login
register
mail settings
Submitter Jeroen Hofstee
Date Feb. 2, 2013, 2:05 p.m.
Message ID <510D1D1E.7080705@myspectrum.nl>
Download mbox | patch
Permalink /patch/217666/
State Not Applicable
Headers show

Comments

Jeroen Hofstee - Feb. 2, 2013, 2:05 p.m.
Hello Albert,

On 02/02/2013 12:32 PM, Albert ARIBAUD wrote:
>
>>>
>>> Sebastian wrote On 01.02.2013 08:55:
>>>> we are using u-boot in our embedded system with arm-1136jfs cpu.
>>>> We recently tried a new toolchain with GCC 4.7.2.
>>>> If compiled with the new toolchain the feature CONFIG_AUTO_COMPLETE isn't working.
> [..] AFAIK it has never been seen on mainline code,
The twister board from mainline / current master also has
this problem. I believe the mt_ventoux will have it as well,
but can't test it.
> and has been very elusive for those who experience it -- enabling debug
> or simply adding/removing code makes it go away or reappear, leading me
> to thinking that some of the added code either does a data abort itself,
> or causes one in the mainline code it calls upon.
yes, it is confusing. The following patch will e.g. make the
trap go away on the twister. Yet there is nothing wrong with the
original code it touches (or I fail to see what it is).

Regards,
Jeroen

         const char *cmd;
@@ -224,11 +223,7 @@ static int complete_cmdv(int argc, char * const 
argv[], char last_char, int maxv
          * Some commands allow length modifiers (like "cp.b");
          * compare command name only until first dot.
          */
-       p = strchr(cmd, '.');
-       if (p == NULL)
-               len = strlen(cmd);
-       else
-               len = p - cmd;
+       len = strlen(cmd);

         /* return the partial matches */
         for (; cmdtp != cmdend; cmdtp++) {
Marek Vasut - Feb. 2, 2013, 3:05 p.m.
Dear Jeroen Hofstee,

> Hello Albert,
> 
> On 02/02/2013 12:32 PM, Albert ARIBAUD wrote:
> >>> Sebastian wrote On 01.02.2013 08:55:
> >>>> we are using u-boot in our embedded system with arm-1136jfs cpu.
> >>>> We recently tried a new toolchain with GCC 4.7.2.
> >>>> If compiled with the new toolchain the feature CONFIG_AUTO_COMPLETE
> >>>> isn't working.
> > 
> > [..] AFAIK it has never been seen on mainline code,
> 
> The twister board from mainline / current master also has
> this problem. I believe the mt_ventoux will have it as well,
> but can't test it.
> 
> > and has been very elusive for those who experience it -- enabling debug
> > or simply adding/removing code makes it go away or reappear, leading me
> > to thinking that some of the added code either does a data abort itself,
> > or causes one in the mainline code it calls upon.
> 
> yes, it is confusing. The following patch will e.g. make the
> trap go away on the twister. Yet there is nothing wrong with the
> original code it touches (or I fail to see what it is).
> 
> Regards,
> Jeroen
> 
> diff --git a/common/command.c b/common/command.c
> index 50c8429..520bd39 100644
> --- a/common/command.c
> +++ b/common/command.c
> @@ -185,7 +185,6 @@ static int complete_cmdv(int argc, char * const
> argv[], char last_char, int maxv
>          cmd_tbl_t *cmdtp = ll_entry_start(cmd_tbl_t, cmd);
>          const int count = ll_entry_count(cmd_tbl_t, cmd);
>          const cmd_tbl_t *cmdend = cmdtp + count;
> -       const char *p;
>          int len, clen;
>          int n_found = 0;
>          const char *cmd;
> @@ -224,11 +223,7 @@ static int complete_cmdv(int argc, char * const
> argv[], char last_char, int maxv
>           * Some commands allow length modifiers (like "cp.b");
>           * compare command name only until first dot.
>           */
> -       p = strchr(cmd, '.');
> -       if (p == NULL)
> -               len = strlen(cmd);
> -       else
> -               len = p - cmd;
> +       len = strlen(cmd);
> 
>          /* return the partial matches */
>          for (; cmdtp != cmdend; cmdtp++) {

Could it be that 'cmd' is possibly not zero-terminated string?

Best regards,
Marek Vasut
Wolfgang Denk - Feb. 2, 2013, 9:22 p.m.
Dear Jeroen Hofstee,

In message <510D1D1E.7080705@myspectrum.nl> you wrote:
> 
> yes, it is confusing. The following patch will e.g. make the
> trap go away on the twister. Yet there is nothing wrong with the
> original code it touches (or I fail to see what it is).

Note that your patch breaks commands that use length modifiers ...

Best regards,

Wolfgang Denk
Wolfgang Denk - Feb. 2, 2013, 9:23 p.m.
Dear Marek Vasut,

In message <201302021605.21681.marex@denx.de> you wrote:
> 
> Could it be that 'cmd' is possibly not zero-terminated string?

How would that ever happen?

Best regards,

Wolfgang Denk
Jeroen Hofstee - Feb. 2, 2013, 9:44 p.m.
On 02/02/2013 10:22 PM, Wolfgang Denk wrote:
> Dear Jeroen Hofstee,
>
> In message <510D1D1E.7080705@myspectrum.nl> you wrote:
>> yes, it is confusing. The following patch will e.g. make the
>> trap go away on the twister. Yet there is nothing wrong with the
>> original code it touches (or I fail to see what it is).
> Note that your patch breaks commands that use length modifiers ...
>
> Best regards,
>
> Wolfgang Denk
>
I'm aware of that. This is not a patch to be applied, just to illustrate
the weirdness encountered (it explicitly says the code is fine before
this). This is resolved for now, see

http://patchwork.ozlabs.org/patch/217695/

Regards,
Jeroen
Priebe, Sebastian - Feb. 4, 2013, 7:11 a.m.
Hello,

> So it seems to be, that patch at least solves this issue.
> Sebastian: can you check if this is resolved also resolved for your board after applying http://patchwork.ozlabs.org/patch/217695/

Apperently we are still working with v2012.10. Could someone be so kind and provide a patch for v2012.10?
We plan to upgrade to v2013.01, but not before the end of Februay.

> Then this smells like a tool chain issue.  You might contact Pengutronix support for help with their tool chain.

We already asked Pengutronix.
They use barebox with their toolchains and didn't have any problem with their new toolchain, yet.
In their barebox.lds they have:
 __barebox_cmd_start = .;
 __barebox_cmd : { KEEP(*(SORT_BY_NAME(.barebox_cmd*))) }
 __barebox_cmd_end = .;

And they thought
         __u_boot_cmd_start = .;
        .u_boot_cmd : { KEEP(*(.u_boot_cmd)) }
        __u_boot_cmd_end = .;

would solve the problem. But it didn't.

Best regards.
Sebastian




==========================================
CADCON
Ingenieurgesellschaft mbH & Co. KG
Geschaeftsfuehrer: Robert Bauer, Andreas Gundel
Sitz der Gesellschaft: D-86368 Gersthofen
Registergericht: Amtsgericht Augsburg HRA 14521
==========================================
-----Ursprüngliche Nachricht-----
Von: Jeroen Hofstee [mailto:jeroen@myspectrum.nl]
Gesendet: Samstag, 2. Februar 2013 22:45
An: Wolfgang Denk
Cc: Jeroen Hofstee; Marek Vasut; Heiko Schocher; Priebe, Sebastian; u-boot@lists.denx.de
Betreff: Re: [U-Boot] U-Boot Bug with newer GCC

On 02/02/2013 10:22 PM, Wolfgang Denk wrote:
> Dear Jeroen Hofstee,
>
> In message <510D1D1E.7080705@myspectrum.nl> you wrote:
>> yes, it is confusing. The following patch will e.g. make the trap go
>> away on the twister. Yet there is nothing wrong with the original
>> code it touches (or I fail to see what it is).
> Note that your patch breaks commands that use length modifiers ...
>
> Best regards,
>
> Wolfgang Denk
>
I'm aware of that. This is not a patch to be applied, just to illustrate the weirdness encountered (it explicitly says the code is fine before this). This is resolved for now, see

http://patchwork.ozlabs.org/patch/217695/

Regards,
Jeroen
Albert ARIBAUD - Feb. 4, 2013, 8:49 a.m.
Hi Sebastian,

On Mon, 4 Feb 2013 07:11:30 +0000, "Priebe, Sebastian"
<Sebastian.Priebe@cadcon.de> wrote:

> Hello,
> 
> > So it seems to be, that patch at least solves this issue.
> > Sebastian: can you check if this is resolved also resolved for your board after applying http://patchwork.ozlabs.org/patch/217695/
> 
> Apperently we are still working with v2012.10. Could someone be so kind and provide a patch for v2012.10?
> We plan to upgrade to v2013.01, but not before the end of Februay.

Did you try to 'apply --reject' the patch to 2012.10 and see how this
goes?

> > Then this smells like a tool chain issue.  You might contact Pengutronix support for help with their tool chain.
> 
> We already asked Pengutronix.
> They use barebox with their toolchains and didn't have any problem with their new toolchain, yet.
> In their barebox.lds they have:
>  __barebox_cmd_start = .;
>  __barebox_cmd : { KEEP(*(SORT_BY_NAME(.barebox_cmd*))) }
>  __barebox_cmd_end = .;
> 
> And they thought
>          __u_boot_cmd_start = .;
>         .u_boot_cmd : { KEEP(*(.u_boot_cmd)) }
>         __u_boot_cmd_end = .;
> 
> would solve the problem. But it didn't.

As long as symbols are defined at linker level it won't, I guess. My
patch actively changes the way the commands start and end symbols are
defined.

> Best regards.
> Sebastian

Amicalement,
Priebe, Sebastian - Feb. 4, 2013, 11:28 a.m.
Hello,

> Did you try to 'apply --reject' the patch to 2012.10 and see how this goes?

This only adds a newline in command.c.

Regards,
Sebastian





==========================================
CADCON
Ingenieurgesellschaft mbH & Co. KG
Geschaeftsfuehrer: Robert Bauer, Andreas Gundel
Sitz der Gesellschaft: D-86368 Gersthofen
Registergericht: Amtsgericht Augsburg HRA 14521
==========================================
-----Ursprüngliche Nachricht-----
Von: Albert ARIBAUD [mailto:albert.u.boot@aribaud.net]
Gesendet: Montag, 4. Februar 2013 09:49
An: Priebe, Sebastian
Cc: Jeroen Hofstee; Wolfgang Denk; Marek Vasut; u-boot@lists.denx.de; Heiko Schocher
Betreff: Re: [U-Boot] U-Boot Bug with newer GCC

Hi Sebastian,

On Mon, 4 Feb 2013 07:11:30 +0000, "Priebe, Sebastian"
<Sebastian.Priebe@cadcon.de> wrote:

> Hello,
>
> > So it seems to be, that patch at least solves this issue.
> > Sebastian: can you check if this is resolved also resolved for your
> > board after applying http://patchwork.ozlabs.org/patch/217695/
>
> Apperently we are still working with v2012.10. Could someone be so kind and provide a patch for v2012.10?
> We plan to upgrade to v2013.01, but not before the end of Februay.

Did you try to 'apply --reject' the patch to 2012.10 and see how this goes?

> > Then this smells like a tool chain issue.  You might contact Pengutronix support for help with their tool chain.
>
> We already asked Pengutronix.
> They use barebox with their toolchains and didn't have any problem with their new toolchain, yet.
> In their barebox.lds they have:
>  __barebox_cmd_start = .;
>  __barebox_cmd : { KEEP(*(SORT_BY_NAME(.barebox_cmd*))) }
> __barebox_cmd_end = .;
>
> And they thought
>          __u_boot_cmd_start = .;
>         .u_boot_cmd : { KEEP(*(.u_boot_cmd)) }
>         __u_boot_cmd_end = .;
>
> would solve the problem. But it didn't.

As long as symbols are defined at linker level it won't, I guess. My patch actively changes the way the commands start and end symbols are defined.

> Best regards.
> Sebastian

Amicalement,
--
Albert.
Albert ARIBAUD - Feb. 4, 2013, 11:35 a.m.
Hi Sebastian,

On Mon, 4 Feb 2013 11:28:26 +0000, "Priebe, Sebastian"
<Sebastian.Priebe@cadcon.de> wrote:

> Hello,
> 
> > Did you try to 'apply --reject' the patch to 2012.10 and see how this goes?
> 
> This only adds a newline in command.c.

Do you mean the only change you see at all is this new line addition? I
suspect there are other changes, which you missed, maybe because they
apply fine. What does a 'git status' produce?

Amicalement,
Priebe, Sebastian - Feb. 4, 2013, 2:23 p.m.
Hello,

> Do you mean the only change you see at all is this new line addition? I suspect there are other changes, which you missed, maybe because they apply fine. What does a 'git status' produce?

We are using svn, not git. There are too many differences. I can't apply the patch. There is no e.g. linker_list.h, etc...
svn status after patching show only command/command.c changed.

Greetings,
Sebastian





==========================================
CADCON
Ingenieurgesellschaft mbH & Co. KG
Geschaeftsfuehrer: Robert Bauer, Andreas Gundel
Sitz der Gesellschaft: D-86368 Gersthofen
Registergericht: Amtsgericht Augsburg HRA 14521
==========================================
-----Ursprüngliche Nachricht-----
Von: Albert ARIBAUD [mailto:albert.u.boot@aribaud.net]
Gesendet: Montag, 4. Februar 2013 12:36
An: Priebe, Sebastian
Cc: Jeroen Hofstee; Wolfgang Denk; Marek Vasut; u-boot@lists.denx.de; Heiko Schocher
Betreff: Re: [U-Boot] U-Boot Bug with newer GCC

Hi Sebastian,

On Mon, 4 Feb 2013 11:28:26 +0000, "Priebe, Sebastian"
<Sebastian.Priebe@cadcon.de> wrote:

> Hello,
>
> > Did you try to 'apply --reject' the patch to 2012.10 and see how this goes?
>
> This only adds a newline in command.c.

Do you mean the only change you see at all is this new line addition? I suspect there are other changes, which you missed, maybe because they apply fine. What does a 'git status' produce?

Amicalement,
--
Albert.
Albert ARIBAUD - Feb. 4, 2013, 2:32 p.m.
Hi Sebastian,

On Mon, 4 Feb 2013 14:23:17 +0000, "Priebe, Sebastian"
<Sebastian.Priebe@cadcon.de> wrote:

> Hello,
> 
> > Do you mean the only change you see at all is this new line addition? I suspect there are other changes, which you missed, maybe because they apply fine. What does a 'git status' produce?
> 
> We are using svn, not git. There are too many differences. I can't apply the patch. There is no e.g. linker_list.h, etc...
> svn status after patching show only command/command.c changed.

(maybe you should set your mailing tool to wrap at ~70 characters)

If there is no linker_list.h, then the patch is basically not
applicable -- you'd have to hand-port it by manually rewriting the
linker file to include sections instead of defining symbols, something
like

         .u_boot_cmd_start : { KEEP(*(.u_boot_cmd_start)) }
        .u_boot_cmd : { KEEP(*(.u_boot_cmd)) }
         .u_boot_cmd_end : { KEEP(*(.u_boot_cmd_end)) }

And then, somewhere in a C file, define __u_boot_cmd_start
as a 'struct {}' placed in section .u_boot_cmd_start, and
__u_boot_cmd_end as a struct {} placed in section .u_boot_cmd_end.

> Greetings,
> Sebastian

Amicalement,
Priebe, Sebastian - Feb. 4, 2013, 3:32 p.m.
Hello,

> (maybe you should set your mailing tool to wrap at ~70 characters)

Apperently I can't. The mailing tool (MS Outlook) setting are restricted by my companys IT.
I have no "root" rights on my windows machine. I develop only on my linux machine.

> If there is no linker_list.h, then the patch is basically not applicable -- you'd have to hand-port it by manually rewriting the linker file to include sections instead of defining symbols, something like

I will wait until we upgraded to v2013.01 and apply the patch then. The problem seems to be solved, as several mailings said.
I will work with the old toolchain until our upgrade. If the problem still exists after the upgrade I will scream again ;-)

Thanks for the fast support.
Sebastian




==========================================
CADCON
Ingenieurgesellschaft mbH & Co. KG
Geschaeftsfuehrer: Robert Bauer, Andreas Gundel
Sitz der Gesellschaft: D-86368 Gersthofen
Registergericht: Amtsgericht Augsburg HRA 14521
==========================================
-----Ursprüngliche Nachricht-----
Von: Albert ARIBAUD [mailto:albert.u.boot@aribaud.net]
Gesendet: Montag, 4. Februar 2013 15:32
An: Priebe, Sebastian
Cc: Jeroen Hofstee; Wolfgang Denk; Marek Vasut; u-boot@lists.denx.de; Heiko Schocher
Betreff: Re: [U-Boot] U-Boot Bug with newer GCC

Hi Sebastian,

On Mon, 4 Feb 2013 14:23:17 +0000, "Priebe, Sebastian"
<Sebastian.Priebe@cadcon.de> wrote:

> Hello,
>
> > Do you mean the only change you see at all is this new line addition? I suspect there are other changes, which you missed, maybe because they apply fine. What does a 'git status' produce?
>
> We are using svn, not git. There are too many differences. I can't apply the patch. There is no e.g. linker_list.h, etc...
> svn status after patching show only command/command.c changed.

(maybe you should set your mailing tool to wrap at ~70 characters)

If there is no linker_list.h, then the patch is basically not applicable -- you'd have to hand-port it by manually rewriting the linker file to include sections instead of defining symbols, something like

         .u_boot_cmd_start : { KEEP(*(.u_boot_cmd_start)) }
        .u_boot_cmd : { KEEP(*(.u_boot_cmd)) }
         .u_boot_cmd_end : { KEEP(*(.u_boot_cmd_end)) }

And then, somewhere in a C file, define __u_boot_cmd_start as a 'struct {}' placed in section .u_boot_cmd_start, and __u_boot_cmd_end as a struct {} placed in section .u_boot_cmd_end.

> Greetings,
> Sebastian

Amicalement,
--
Albert.
Wolfgang Denk - Feb. 4, 2013, 8:49 p.m.
Dear "Priebe, Sebastian",

In message <E70AF999396FDF4EAE40E195B847096109F5F87F@SRVEXCH-2K10.CADCON.INTERN> you wrote:
> 
> > (maybe you should set your mailing tool to wrap at ~70 characters)
> 
> Apperently I can't. The mailing tool (MS Outlook) setting are restricted by=
>  my companys IT.
> I have no "root" rights on my windows machine. I develop only on my linux m=
> achine.

Don't use this, then, to communicate with public mailing lists.

There are many other options available, including many free ones.

Best regards,

Wolfgang Denk

Patch

diff --git a/common/command.c b/common/command.c
index 50c8429..520bd39 100644
--- a/common/command.c
+++ b/common/command.c
@@ -185,7 +185,6 @@  static int complete_cmdv(int argc, char * const 
argv[], char last_char, int maxv
         cmd_tbl_t *cmdtp = ll_entry_start(cmd_tbl_t, cmd);
         const int count = ll_entry_count(cmd_tbl_t, cmd);
         const cmd_tbl_t *cmdend = cmdtp + count;
-       const char *p;
         int len, clen;
         int n_found = 0;