| Message ID | 1291100800-61850-4-git-send-email-andreas.devel@googlemail.com |
|---|---|
| State | RFC |
| Headers | show |
Le 30/11/2010 08:06, Andreas Bießmann a écrit : > Signed-off-by: Andreas Bießmann<andreas.devel@googlemail.com> > + cmp r1, #0 /* symbol == NULL ? */ > + beq fixnext Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since they are a constant. If a NULL ends up in relocation tables, that is because of a corruption *or* because it was to be relocated, and should thus never be ignored. Amicalement,
> > Le 30/11/2010 08:06, Andreas Bießmann a écrit : > > Signed-off-by: Andreas Bießmann<andreas.devel@googlemail.com> > > > + cmp r1, #0 /* symbol == NULL ? */ > > + beq fixnext > > Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since > they are a constant. If a NULL ends up in relocation tables, that is > because of a corruption *or* because it was to be relocated, and should > thus never be ignored. Depends, if the same routine is used for relocating fixups you need this test. Undefined weaks will generate a NULL fixup entry. Jocke
Dear All, Am 30.11.2010 09:47, schrieb Joakim Tjernlund: >> >> Le 30/11/2010 08:06, Andreas Bießmann a écrit : >>> Signed-off-by: Andreas Bießmann<andreas.devel@googlemail.com> >> >>> + cmp r1, #0 /* symbol == NULL ? */ >>> + beq fixnext >> >> Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since >> they are a constant. If a NULL ends up in relocation tables, that is >> because of a corruption *or* because it was to be relocated, and should >> thus never be ignored. > > Depends, if the same routine is used for relocating fixups you need > this test. > Undefined weaks will generate a NULL fixup entry. As mentioned by Jens and one other some time ago. Albert, please build e.g. at91rm9200ek boards and search for board_reset(), defined in arch/arm/cpu/arm920t/at91/reset.c regards Andreas Bießmann
Le 30/11/2010 09:47, Joakim Tjernlund a écrit : >> >> Le 30/11/2010 08:06, Andreas Bießmann a écrit : >>> Signed-off-by: Andreas Bießmann<andreas.devel@googlemail.com> >> >>> + cmp r1, #0 /* symbol == NULL ? */ >>> + beq fixnext >> >> Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since >> they are a constant. If a NULL ends up in relocation tables, that is >> because of a corruption *or* because it was to be relocated, and should >> thus never be ignored. > > Depends, if the same routine is used for relocating fixups you need > this test. Undefined weaks will generate a NULL fixup entry. Understood. Weren't there an effort to not use weak symbols any more? If not, then a comment *must* be added to indicate that weak symbols can cause zero-filled reloc entries (I would suggest saying 'zero-filled' rather than 'NULL', because obviously readers will think of stdio's NULL). We could possibly even send out a diagnostic message, but that'll be when the relocation code is turned to C language; I don't want to see asm code that calls printf. > Jocke Amicalement,
Albert ARIBAUD <albert@aribaud.net> wrote on 2010/11/30 10:02:45: > > Le 30/11/2010 09:47, Joakim Tjernlund a écrit : > >> > >> Le 30/11/2010 08:06, Andreas Bießmann a écrit : > >>> Signed-off-by: Andreas Bießmann<andreas.devel@googlemail.com> > >> > >>> + cmp r1, #0 /* symbol == NULL ? */ > >>> + beq fixnext > >> > >> Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since > >> they are a constant. If a NULL ends up in relocation tables, that is > >> because of a corruption *or* because it was to be relocated, and should > >> thus never be ignored. > > > > Depends, if the same routine is used for relocating fixups you need > > this test. Undefined weaks will generate a NULL fixup entry. > > Understood. note that I don't know how this routine is used so if just relocates the GOT you don't need to test for NULL. > > Weren't there an effort to not use weak symbols any more? ehh, not what I am aware of. Just that we should not use(ATM) undefined weaks. > > If not, then a comment *must* be added to indicate that weak symbols can > cause zero-filled reloc entries (I would suggest saying 'zero-filled' > rather than 'NULL', because obviously readers will think of stdio's NULL). zero-filled/NULL fixup entries. The GOT never holds NULL > > We could possibly even send out a diagnostic message, but that'll be > when the relocation code is turned to C language; I don't want to see > asm code that calls printf. No need really.
Dear Joakim Tjernlund, Dear Albert ARIBAUD, Am 30.11.2010 10:41, schrieb Joakim Tjernlund: > Albert ARIBAUD <albert@aribaud.net> wrote on 2010/11/30 10:02:45: >> >> Le 30/11/2010 09:47, Joakim Tjernlund a écrit : >>>> >>>> Le 30/11/2010 08:06, Andreas Bießmann a écrit : >>>>> Signed-off-by: Andreas Bießmann<andreas.devel@googlemail.com> >>>> >>>>> + cmp r1, #0 /* symbol == NULL ? */ >>>>> + beq fixnext >>>> >>>> Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since >>>> they are a constant. If a NULL ends up in relocation tables, that is >>>> because of a corruption *or* because it was to be relocated, and should >>>> thus never be ignored. >>> >>> Depends, if the same routine is used for relocating fixups you need >>> this test. Undefined weaks will generate a NULL fixup entry. >> >> Understood. > > note that I don't know how this routine is used so if just > relocates the GOT you don't need to test for NULL. > >> >> Weren't there an effort to not use weak symbols any more? > > ehh, not what I am aware of. Just that we should not use(ATM) > undefined weaks. Ok, I did forget to investigate that as mentioned in http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/88024/focus=88324 Here are the results: Currently arm920/at91 devices have one undefined weak symbol in .dynsym: ---8<--- Symbol table '.dynsym' contains 11 entries: Num: Value Size Type Bind Vis Ndx Name 0: 00000000 0 NOTYPE LOCAL DEFAULT UND <corrupt> 1: 10000000 0 SECTION LOCAL DEFAULT 1 <corrupt> 2: 10028a28 0 SECTION LOCAL DEFAULT 6 <corrupt> 3: 10029ff8 0 NOTYPE GLOBAL DEFAULT 9 <corrupt> 4: 100299f4 0 NOTYPE GLOBAL DEFAULT ABS <corrupt> 5: 00000000 0 NOTYPE WEAK DEFAULT UND <corrupt> 6: 10029ff8 0 NOTYPE GLOBAL DEFAULT ABS <corrupt> 7: 10029ff8 0 NOTYPE GLOBAL DEFAULT 11 <corrupt> 8: 1002edf0 0 NOTYPE GLOBAL DEFAULT 10 <corrupt> 9: 100701c0 0 NOTYPE GLOBAL DEFAULT 11 <corrupt> 10: 1002edf0 0 NOTYPE GLOBAL DEFAULT 9 <corrupt> --->8--- ---8<--- Hex dump of section '.dynsym': 0x1002edf0 00000000 00000000 00000000 00000000 ................ 0x1002ee00 00000000 00000010 00000000 03000100 ................ 0x1002ee10 00000000 288a0210 00000000 03000600 ....(........... 0x1002ee20 25000000 f89f0210 00000000 10000900 %............... 0x1002ee30 01000000 f4990210 00000000 1000f1ff ................ 0x1002ee40 5e000000 00000000 00000000 20000000 ^........... ... 0x1002ee50 14000000 f89f0210 00000000 1000f1ff ................ 0x1002ee60 52000000 f89f0210 00000000 10000b00 R............... 0x1002ee70 43000000 f0ed0210 00000000 10000a00 C............... 0x1002ee80 20000000 c0010710 00000000 10000b00 ............... 0x1002ee90 35000000 f0ed0210 00000000 10000900 5............... --->8--- So there are two options here. First is to apply '[PATCH v2 1/2] arm920t/at91/reset.c: fix weak reset_board()' second (and safer) is to apply (maybe modified) '[PATCH RFC 3/3] arm920t: do not relocate NULL pointer'. which one is preferred? Or should we do both? regards Andreas Bießmann
"Andreas Bießmann" <andreas.devel@googlemail.com> wrote on 2010/11/30 12:48:41: > > Dear Joakim Tjernlund, > Dear Albert ARIBAUD, > > Am 30.11.2010 10:41, schrieb Joakim Tjernlund: > > Albert ARIBAUD <albert@aribaud.net> wrote on 2010/11/30 10:02:45: > >> > >> Le 30/11/2010 09:47, Joakim Tjernlund a écrit : > >>>> > >>>> Le 30/11/2010 08:06, Andreas Bießmann a écrit : > >>>>> Signed-off-by: Andreas Bießmann<andreas.devel@googlemail.com> > >>>> > >>>>> + cmp r1, #0 /* symbol == NULL ? */ > >>>>> + beq fixnext > >>>> > >>>> Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since > >>>> they are a constant. If a NULL ends up in relocation tables, that is > >>>> because of a corruption *or* because it was to be relocated, and should > >>>> thus never be ignored. > >>> > >>> Depends, if the same routine is used for relocating fixups you need > >>> this test. Undefined weaks will generate a NULL fixup entry. > >> > >> Understood. > > > > note that I don't know how this routine is used so if just > > relocates the GOT you don't need to test for NULL. > > > >> > >> Weren't there an effort to not use weak symbols any more? > > > > ehh, not what I am aware of. Just that we should not use(ATM) > > undefined weaks. > > Ok, I did forget to investigate that as mentioned in > http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/88024/focus=88324 > > Here are the results: > > Currently arm920/at91 devices have one undefined weak symbol in .dynsym: > > ---8<--- > Symbol table '.dynsym' contains 11 entries: > Num: Value Size Type Bind Vis Ndx Name > 0: 00000000 0 NOTYPE LOCAL DEFAULT UND <corrupt> > 1: 10000000 0 SECTION LOCAL DEFAULT 1 <corrupt> > 2: 10028a28 0 SECTION LOCAL DEFAULT 6 <corrupt> > 3: 10029ff8 0 NOTYPE GLOBAL DEFAULT 9 <corrupt> > 4: 100299f4 0 NOTYPE GLOBAL DEFAULT ABS <corrupt> > 5: 00000000 0 NOTYPE WEAK DEFAULT UND <corrupt> > 6: 10029ff8 0 NOTYPE GLOBAL DEFAULT ABS <corrupt> > 7: 10029ff8 0 NOTYPE GLOBAL DEFAULT 11 <corrupt> > 8: 1002edf0 0 NOTYPE GLOBAL DEFAULT 10 <corrupt> > 9: 100701c0 0 NOTYPE GLOBAL DEFAULT 11 <corrupt> > 10: 1002edf0 0 NOTYPE GLOBAL DEFAULT 9 <corrupt> > --->8--- > > ---8<--- > Hex dump of section '.dynsym': > 0x1002edf0 00000000 00000000 00000000 00000000 ................ > 0x1002ee00 00000000 00000010 00000000 03000100 ................ > 0x1002ee10 00000000 288a0210 00000000 03000600 ....(........... > 0x1002ee20 25000000 f89f0210 00000000 10000900 %............... > 0x1002ee30 01000000 f4990210 00000000 1000f1ff ................ > 0x1002ee40 5e000000 00000000 00000000 20000000 ^........... ... > 0x1002ee50 14000000 f89f0210 00000000 1000f1ff ................ > 0x1002ee60 52000000 f89f0210 00000000 10000b00 R............... > 0x1002ee70 43000000 f0ed0210 00000000 10000a00 C............... > 0x1002ee80 20000000 c0010710 00000000 10000b00 ............... > 0x1002ee90 35000000 f0ed0210 00000000 10000900 5............... > --->8--- > > So there are two options here. > > First is to apply '[PATCH v2 1/2] arm920t/at91/reset.c: fix weak > reset_board()' second (and safer) is to apply (maybe modified) '[PATCH > RFC 3/3] arm920t: do not relocate NULL pointer'. > > which one is preferred? Or should we do both? Both, you don't know what the future will be and perhaps someone wants to use undef weaks in board code. Jocke
diff --git a/arch/arm/cpu/arm920t/start.S b/arch/arm/cpu/arm920t/start.S index 629be3f..e3f9cdb 100644 --- a/arch/arm/cpu/arm920t/start.S +++ b/arch/arm/cpu/arm920t/start.S @@ -248,11 +248,15 @@ fixabs: mov r1, r1, LSR #4 /* r1 <- symbol index in .dynsym */ add r1, r10, r1 /* r1 <- address of symbol in table */ ldr r1, [r1, #4] /* r1 <- symbol value */ + cmp r1, #0 /* symbol == NULL ? */ + beq fixnext add r1, r9 /* r1 <- relocated sym addr */ b fixnext fixrel: /* relative fix: increase location by offset */ ldr r1, [r0] /* r1 <- address of symbol */ + cmp r1, #0 /* symbol == NULL ? */ + beq fixnext add r1, r1, r9 /* r1 <- relocated address of symbol */ fixnext: str r1, [r0] /* store back content of r1 */
Signed-off-by: Andreas Bießmann <andreas.devel@googlemail.com> --- arch/arm/cpu/arm920t/start.S | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)