* [PATCH] ARM: panic with a message when relocation type is unknown
@ 2019-05-31 9:02 Ahmad Fatoum
2019-05-31 14:43 ` Sam Ravnborg
2019-05-31 18:56 ` Andrey Smirnov
0 siblings, 2 replies; 7+ messages in thread
From: Ahmad Fatoum @ 2019-05-31 9:02 UTC (permalink / raw)
To: barebox
Currently such failures result in a
>00000000 00000000
### ERROR ### Please RESET the board ###
With this patch this now becomes
>00000000 00000000
Unknown relocation type
### ERROR ### Please RESET the board ###
which improves user experience a little bit.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
arch/arm/cpu/common.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/cpu/common.c b/arch/arm/cpu/common.c
index 821cafbf26c2..3668c5977ca9 100644
--- a/arch/arm/cpu/common.c
+++ b/arch/arm/cpu/common.c
@@ -78,7 +78,7 @@ void relocate_to_current_adr(void)
putc_ll(' ');
puthex_ll(rel->r_addend);
putc_ll('\n');
- panic("");
+ panic("Unknown relocation type");
}
dstart += sizeof(*rel);
@@ -108,7 +108,7 @@ void relocate_to_current_adr(void)
putc_ll(' ');
puthex_ll(rel->r_offset);
putc_ll('\n');
- panic("");
+ panic("Unknown relocation type");
}
dstart += sizeof(*rel);
--
2.20.1
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ARM: panic with a message when relocation type is unknown
2019-05-31 9:02 [PATCH] ARM: panic with a message when relocation type is unknown Ahmad Fatoum
@ 2019-05-31 14:43 ` Sam Ravnborg
2019-06-03 5:21 ` Ahmad Fatoum
2019-05-31 18:56 ` Andrey Smirnov
1 sibling, 1 reply; 7+ messages in thread
From: Sam Ravnborg @ 2019-05-31 14:43 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
Hi Ahmad.
On Fri, May 31, 2019 at 11:02:23AM +0200, Ahmad Fatoum wrote:
> Currently such failures result in a
>
> >00000000 00000000
>
> ### ERROR ### Please RESET the board ###
>
> With this patch this now becomes
>
> >00000000 00000000
> Unknown relocation type
> ### ERROR ### Please RESET the board ###
>
> which improves user experience a little bit.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> arch/arm/cpu/common.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/cpu/common.c b/arch/arm/cpu/common.c
> index 821cafbf26c2..3668c5977ca9 100644
> --- a/arch/arm/cpu/common.c
> +++ b/arch/arm/cpu/common.c
> @@ -78,7 +78,7 @@ void relocate_to_current_adr(void)
> putc_ll(' ');
> puthex_ll(rel->r_addend);
> putc_ll('\n');
> - panic("");
> + panic("Unknown relocation type");
> }
>
> dstart += sizeof(*rel);
> @@ -108,7 +108,7 @@ void relocate_to_current_adr(void)
> putc_ll(' ');
> puthex_ll(rel->r_offset);
> putc_ll('\n');
> - panic("");
> + panic("Unknown relocation type");
If you added just a little bit more context then grepping the source
for the error message would only result in one hit.
Like:
"Unknown relocation type (addend)"
"Unknown relocation type (offset)"
Or something like that.
Sam
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ARM: panic with a message when relocation type is unknown
2019-05-31 9:02 [PATCH] ARM: panic with a message when relocation type is unknown Ahmad Fatoum
2019-05-31 14:43 ` Sam Ravnborg
@ 2019-05-31 18:56 ` Andrey Smirnov
2019-06-03 13:03 ` Ahmad Fatoum
1 sibling, 1 reply; 7+ messages in thread
From: Andrey Smirnov @ 2019-05-31 18:56 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: Barebox List
On Fri, May 31, 2019 at 2:02 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Currently such failures result in a
>
> >00000000 00000000
>
> ### ERROR ### Please RESET the board ###
>
> With this patch this now becomes
>
> >00000000 00000000
> Unknown relocation type
> ### ERROR ### Please RESET the board ###
>
> which improves user experience a little bit.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> arch/arm/cpu/common.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/cpu/common.c b/arch/arm/cpu/common.c
> index 821cafbf26c2..3668c5977ca9 100644
> --- a/arch/arm/cpu/common.c
> +++ b/arch/arm/cpu/common.c
> @@ -78,7 +78,7 @@ void relocate_to_current_adr(void)
> putc_ll(' ');
> puthex_ll(rel->r_addend);
> putc_ll('\n');
> - panic("");
> + panic("Unknown relocation type");
> }
>
> dstart += sizeof(*rel);
> @@ -108,7 +108,7 @@ void relocate_to_current_adr(void)
> putc_ll(' ');
> puthex_ll(rel->r_offset);
> putc_ll('\n');
> - panic("");
> + panic("Unknown relocation type");
Looking at this code makes me wonder if calls to panic() are even
appropriate here. I am not sure there's any guarantee that by the time
we get to this line relocation for "Unknown relocation type"(or
original "") string would be processed and panic() would get right
arguments not to mention all of the functions called as a part of
panic() and their potential dependencies. Another thing about painc()
is in PBL it completely ignores passed arguments, so no message will
be printed there.
Maybe it'd make sense to go in a different direction and drop calls to
panic() altogether and replace them with:
puts_ll(relocation_failed_message());
hang();
where relocation_failed_message() on ARM can be something like:
.section .text.relocation_failed_message
ENTRY(relocation_failed_message)
push {lr}
bl 1f
.byte 'U, 'n, 'k, 'n, 'o, 'w, 'n, '\ , 'r, 'e, 'l, 'o, 'c, 'a, 't, 'i,
'o, 'n, '\ , 't, 'y, 'p, 'e, '\r, '\n, 0x00
1:
mov r0, lr
/* In case we are in thumb */
bic r0, r0, #1
pop {pc}
ENDPROC(relocation_failed_message)
to guarantee that it won't depend on relocation? Might be an overkill
though, so take this with a grain of salt.
Thanks,
Andrey Smirnov
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ARM: panic with a message when relocation type is unknown
2019-05-31 14:43 ` Sam Ravnborg
@ 2019-06-03 5:21 ` Ahmad Fatoum
2019-06-03 5:26 ` Ahmad Fatoum
0 siblings, 1 reply; 7+ messages in thread
From: Ahmad Fatoum @ 2019-06-03 5:21 UTC (permalink / raw)
To: Sam Ravnborg; +Cc: barebox
On 31/5/19 16:43, Sam Ravnborg wrote:
> Hi Ahmad.
>
> On Fri, May 31, 2019 at 11:02:23AM +0200, Ahmad Fatoum wrote:
>> Currently such failures result in a
>>
>> >00000000 00000000
>>
>> ### ERROR ### Please RESET the board ###
>>
>> With this patch this now becomes
>>
>> >00000000 00000000
>> Unknown relocation type
>> ### ERROR ### Please RESET the board ###
>>
>> which improves user experience a little bit.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> arch/arm/cpu/common.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/cpu/common.c b/arch/arm/cpu/common.c
>> index 821cafbf26c2..3668c5977ca9 100644
>> --- a/arch/arm/cpu/common.c
>> +++ b/arch/arm/cpu/common.c
>> @@ -78,7 +78,7 @@ void relocate_to_current_adr(void)
>> putc_ll(' ');
>> puthex_ll(rel->r_addend);
>> putc_ll('\n');
>> - panic("");
>> + panic("Unknown relocation type");
>> }
>>
>> dstart += sizeof(*rel);
>> @@ -108,7 +108,7 @@ void relocate_to_current_adr(void)
>> putc_ll(' ');
>> puthex_ll(rel->r_offset);
>> putc_ll('\n');
>> - panic("");
>> + panic("Unknown relocation type");
>
> If you added just a little bit more context then grepping the source
> for the error message would only result in one hit.
>
> Like:
> "Unknown relocation type (addend)"
> "Unknown relocation type (offset)"
Good idea. Will do in v2.
>
> Or something like that.
>
> Sam
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ARM: panic with a message when relocation type is unknown
2019-06-03 5:21 ` Ahmad Fatoum
@ 2019-06-03 5:26 ` Ahmad Fatoum
0 siblings, 0 replies; 7+ messages in thread
From: Ahmad Fatoum @ 2019-06-03 5:26 UTC (permalink / raw)
To: barebox
On 3/6/19 07:21, Ahmad Fatoum wrote:
>
>
> On 31/5/19 16:43, Sam Ravnborg wrote:
>> Hi Ahmad.
>>
>> On Fri, May 31, 2019 at 11:02:23AM +0200, Ahmad Fatoum wrote:
>>> Currently such failures result in a
>>>
>>> >00000000 00000000
>>>
>>> ### ERROR ### Please RESET the board ###
>>>
>>> With this patch this now becomes
>>>
>>> >00000000 00000000
>>> Unknown relocation type
>>> ### ERROR ### Please RESET the board ###
>>>
>>> which improves user experience a little bit.
>>>
>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>> ---
>>> arch/arm/cpu/common.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/cpu/common.c b/arch/arm/cpu/common.c
>>> index 821cafbf26c2..3668c5977ca9 100644
>>> --- a/arch/arm/cpu/common.c
>>> +++ b/arch/arm/cpu/common.c
>>> @@ -78,7 +78,7 @@ void relocate_to_current_adr(void)
>>> putc_ll(' ');
>>> puthex_ll(rel->r_addend);
>>> putc_ll('\n');
>>> - panic("");
>>> + panic("Unknown relocation type");
>>> }
>>>
>>> dstart += sizeof(*rel);
>>> @@ -108,7 +108,7 @@ void relocate_to_current_adr(void)
>>> putc_ll(' ');
>>> puthex_ll(rel->r_offset);
>>> putc_ll('\n');
>>> - panic("");
>>> + panic("Unknown relocation type");
>>
>> If you added just a little bit more context then grepping the source
>> for the error message would only result in one hit.
>>
>> Like:
>> "Unknown relocation type (addend)"
>> "Unknown relocation type (offset)"
>
> Good idea. Will do in v2.
Oh, looking at the full source, it's not really needed. The first panic
is guarded by a #if defined(CONFIG_CPU_64) and the other by
#elif defined(CONFIG_CPU_32).
>
>>
>> Or something like that.
>>
>> Sam
>>
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ARM: panic with a message when relocation type is unknown
2019-05-31 18:56 ` Andrey Smirnov
@ 2019-06-03 13:03 ` Ahmad Fatoum
2019-06-04 6:10 ` Andrey Smirnov
0 siblings, 1 reply; 7+ messages in thread
From: Ahmad Fatoum @ 2019-06-03 13:03 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: Barebox List
Hello Andrey,
On 31/5/19 20:56, Andrey Smirnov wrote:
> On Fri, May 31, 2019 at 2:02 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>>
>> Currently such failures result in a
>>
>> >00000000 00000000
>>
>> ### ERROR ### Please RESET the board ###
>>
>> With this patch this now becomes
>>
>> >00000000 00000000
>> Unknown relocation type
>> ### ERROR ### Please RESET the board ###
>>
>> which improves user experience a little bit.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> arch/arm/cpu/common.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/cpu/common.c b/arch/arm/cpu/common.c
>> index 821cafbf26c2..3668c5977ca9 100644
>> --- a/arch/arm/cpu/common.c
>> +++ b/arch/arm/cpu/common.c
>> @@ -78,7 +78,7 @@ void relocate_to_current_adr(void)
>> putc_ll(' ');
>> puthex_ll(rel->r_addend);
>> putc_ll('\n');
>> - panic("");
>> + panic("Unknown relocation type");
>> }
>>
>> dstart += sizeof(*rel);
>> @@ -108,7 +108,7 @@ void relocate_to_current_adr(void)
>> putc_ll(' ');
>> puthex_ll(rel->r_offset);
>> putc_ll('\n');
>> - panic("");
>> + panic("Unknown relocation type");
>
> Looking at this code makes me wonder if calls to panic() are even
> appropriate here. I am not sure there's any guarantee that by the time
> we get to this line relocation for "Unknown relocation type"(or
> original "") string would be processed and panic() would get right
> arguments not to mention all of the functions called as a part of
> panic() and their potential dependencies. Another thing about painc()
> is in PBL it completely ignores passed arguments, so no message will
> be printed there.
>
> Maybe it'd make sense to go in a different direction and drop calls to
> panic() altogether and replace them with:
>
> puts_ll(relocation_failed_message());
> hang();
>
> where relocation_failed_message() on ARM can be something like:
>
> .section .text.relocation_failed_message
> ENTRY(relocation_failed_message)
> push {lr}
> bl 1f
> .byte 'U, 'n, 'k, 'n, 'o, 'w, 'n, '\ , 'r, 'e, 'l, 'o, 'c, 'a, 't, 'i,
> 'o, 'n, '\ , 't, 'y, 'p, 'e, '\r, '\n, 0x00
> 1:
> mov r0, lr
> /* In case we are in thumb */
> bic r0, r0, #1
> pop {pc}
> ENDPROC(relocation_failed_message)
>
> to guarantee that it won't depend on relocation? Might be an overkill
> though, so take this with a grain of salt.
In my particular case, the string literal was already being accessed relative
to the program counter. Do you know if GCC can be coerced to always do that?
>
> Thanks,
> Andrey Smirnov
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ARM: panic with a message when relocation type is unknown
2019-06-03 13:03 ` Ahmad Fatoum
@ 2019-06-04 6:10 ` Andrey Smirnov
0 siblings, 0 replies; 7+ messages in thread
From: Andrey Smirnov @ 2019-06-04 6:10 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: Barebox List
On Mon, Jun 3, 2019 at 6:03 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Hello Andrey,
>
> On 31/5/19 20:56, Andrey Smirnov wrote:
> > On Fri, May 31, 2019 at 2:02 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> >>
> >> Currently such failures result in a
> >>
> >> >00000000 00000000
> >>
> >> ### ERROR ### Please RESET the board ###
> >>
> >> With this patch this now becomes
> >>
> >> >00000000 00000000
> >> Unknown relocation type
> >> ### ERROR ### Please RESET the board ###
> >>
> >> which improves user experience a little bit.
> >>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >> arch/arm/cpu/common.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/arm/cpu/common.c b/arch/arm/cpu/common.c
> >> index 821cafbf26c2..3668c5977ca9 100644
> >> --- a/arch/arm/cpu/common.c
> >> +++ b/arch/arm/cpu/common.c
> >> @@ -78,7 +78,7 @@ void relocate_to_current_adr(void)
> >> putc_ll(' ');
> >> puthex_ll(rel->r_addend);
> >> putc_ll('\n');
> >> - panic("");
> >> + panic("Unknown relocation type");
> >> }
> >>
> >> dstart += sizeof(*rel);
> >> @@ -108,7 +108,7 @@ void relocate_to_current_adr(void)
> >> putc_ll(' ');
> >> puthex_ll(rel->r_offset);
> >> putc_ll('\n');
> >> - panic("");
> >> + panic("Unknown relocation type");
> >
> > Looking at this code makes me wonder if calls to panic() are even
> > appropriate here. I am not sure there's any guarantee that by the time
> > we get to this line relocation for "Unknown relocation type"(or
> > original "") string would be processed and panic() would get right
> > arguments not to mention all of the functions called as a part of
> > panic() and their potential dependencies. Another thing about painc()
> > is in PBL it completely ignores passed arguments, so no message will
> > be printed there.
> >
> > Maybe it'd make sense to go in a different direction and drop calls to
> > panic() altogether and replace them with:
> >
> > puts_ll(relocation_failed_message());
> > hang();
> >
> > where relocation_failed_message() on ARM can be something like:
> >
> > .section .text.relocation_failed_message
> > ENTRY(relocation_failed_message)
> > push {lr}
> > bl 1f
> > .byte 'U, 'n, 'k, 'n, 'o, 'w, 'n, '\ , 'r, 'e, 'l, 'o, 'c, 'a, 't, 'i,
> > 'o, 'n, '\ , 't, 'y, 'p, 'e, '\r, '\n, 0x00
> > 1:
> > mov r0, lr
> > /* In case we are in thumb */
> > bic r0, r0, #1
> > pop {pc}
> > ENDPROC(relocation_failed_message)
> >
> > to guarantee that it won't depend on relocation? Might be an overkill
> > though, so take this with a grain of salt.
>
> In my particular case, the string literal was already being accessed relative
> to the program counter.
I think that would really be arch dependent. If you were testing on
AArch64, which is much better about PC-relative addressing, then yeah,
that's what I'd expect. In the case of ARM however what we get is
(RDU2 in my case):
7ad1a: 4805 ldr r0, [pc, #20] ; (7ad30 <relocate_to_current_adr+0x88>)
7ad1c: f786 fa5c bl 11d8 <panic>
7ad20: 000f9df0 .word 0x000f9df0
7ad24: 000f9e20 .word 0x000f9e20
7ad28: 000d4600 .word 0x000d4600
7ad2c: 000f9df0 .word 0x000f9df0
7ad30: 000a26be .word 0x000a26be
as well as a corresponding relocation:
arm-none-eabi-objdump -R barebox | grep 7ad30
0007ad30 R_ARM_RELATIVE *ABS*
so, AFAICT, it's definitely going to have to be processed before it
can be used. More so, even if the right string address is passed, we
still would have to make sure that all of the functions that
implementation of panic() is using are not a subject to the same
problem. I think _that_ is a much bigger problem and that's why I
think we should move away from using panic() in that particular part
of the code base.
> Do you know if GCC can be coerced to always do that?
I don't think there's a way (to my knowledge, of course) and I think
on ARM you'd be facing a fundamental limitation on how far PC-relative
addressing can reach. I tried placing the string right after the
function with __attribute__((section(".text.relocate_to_current_adr.data")))
but GCC didn't really pick it up and still used absolute address
constant.
Thanks,
Andrey Smirnov
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-06-04 6:10 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-31 9:02 [PATCH] ARM: panic with a message when relocation type is unknown Ahmad Fatoum
2019-05-31 14:43 ` Sam Ravnborg
2019-06-03 5:21 ` Ahmad Fatoum
2019-06-03 5:26 ` Ahmad Fatoum
2019-05-31 18:56 ` Andrey Smirnov
2019-06-03 13:03 ` Ahmad Fatoum
2019-06-04 6:10 ` Andrey Smirnov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox