* [PATCH 0/1] Fix segfault in DTC
@ 2012-09-24 22:48 Markus Mayer
2012-09-24 22:48 ` [PATCH 1/1] " Markus Mayer
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: Markus Mayer @ 2012-09-24 22:48 UTC (permalink / raw)
To: linux-arm-kernel
Please let me know if there is a different place this patch should be sent
to. Thanks.
Markus Mayer (1):
Fix segfault in dtc when empty input file is given.
scripts/dtc/treesource.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
--
1.7.5.4
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH 1/1] Fix segfault in DTC 2012-09-24 22:48 [PATCH 0/1] Fix segfault in DTC Markus Mayer @ 2012-09-24 22:48 ` Markus Mayer 2012-09-25 11:07 ` [PATCH 0/1] " Will Deacon 2012-09-25 17:58 ` Markus Mayer 2 siblings, 0 replies; 22+ messages in thread From: Markus Mayer @ 2012-09-24 22:48 UTC (permalink / raw) To: linux-arm-kernel Prior to this change, an empty input file would cause a segfault, because yylloc had never been initialized. There was never any characters for the lexer to match, so YY_USER_ACTION was never executed before the parse error was detected. When the parser printed the error message, it tried to include the name of the file, but the structure holding the file name (yylloc.file, referenced as pos->file) had never been initialized. Without the fix: $ ./dtc /dev/null DTC: dts->dts on file "/dev/null" Segmentation fault (core dumped) $ gdb dtc core Program terminated with signal 11, Segmentation fault. at scripts/dtc/srcpos.c:194 194 fname = pos->file->name; (gdb) bt at scripts/dtc/srcpos.c:194 fmt=0x40d769 "%s", va=0x7fffbf027148) at scripts/dtc/srcpos.c:220 at scripts/dtc/dtc-parser.tab.c:1920 at scripts/dtc/treesource.c:38 at scripts/dtc/dtc.c:203 (gdb) p *pos $1 = {first_line = 0, first_column = 0, last_line = 0, last_column = 0, file = 0x0} With the fix: $ ./dtc /dev/null DTC: dts->dts on file "/dev/null" Error: /dev/null:1.1 syntax error FATAL ERROR: Unable to parse input tree Signed-off-by: Markus Mayer <mmayer@broadcom.com> --- scripts/dtc/treesource.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/scripts/dtc/treesource.c b/scripts/dtc/treesource.c index c09aafa..b461b88 100644 --- a/scripts/dtc/treesource.c +++ b/scripts/dtc/treesource.c @@ -29,11 +29,15 @@ int treesource_error; struct boot_info *dt_from_source(const char *fname) { + extern YYLTYPE yylloc; + the_boot_info = NULL; treesource_error = 0; srcfile_push(fname); yyin = current_srcfile->f; + /* Initialize yylloc->file to avoid segfault on empty input */ + srcpos_update(&yylloc, NULL, 0); if (yyparse() != 0) die("Unable to parse input tree\n"); -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 0/1] Fix segfault in DTC 2012-09-24 22:48 [PATCH 0/1] Fix segfault in DTC Markus Mayer 2012-09-24 22:48 ` [PATCH 1/1] " Markus Mayer @ 2012-09-25 11:07 ` Will Deacon 2012-09-25 15:44 ` Stephen Warren 2012-09-25 17:58 ` Markus Mayer 2 siblings, 1 reply; 22+ messages in thread From: Will Deacon @ 2012-09-25 11:07 UTC (permalink / raw) To: linux-arm-kernel On Mon, Sep 24, 2012 at 11:48:04PM +0100, Markus Mayer wrote: > Please let me know if there is a different place this patch should be sent > to. Thanks. You could probably CC devicetree-discuss and also David Gibson for patches against dtc. Will ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 0/1] Fix segfault in DTC 2012-09-25 11:07 ` [PATCH 0/1] " Will Deacon @ 2012-09-25 15:44 ` Stephen Warren 2012-09-25 16:42 ` Markus Mayer 0 siblings, 1 reply; 22+ messages in thread From: Stephen Warren @ 2012-09-25 15:44 UTC (permalink / raw) To: linux-arm-kernel On 09/25/2012 05:07 AM, Will Deacon wrote: > On Mon, Sep 24, 2012 at 11:48:04PM +0100, Markus Mayer wrote: >> Please let me know if there is a different place this patch should be sent >> to. Thanks. > > You could probably CC devicetree-discuss and also David Gibson for patches > against dtc. And Jon Loeliger. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 0/1] Fix segfault in DTC 2012-09-25 15:44 ` Stephen Warren @ 2012-09-25 16:42 ` Markus Mayer 0 siblings, 0 replies; 22+ messages in thread From: Markus Mayer @ 2012-09-25 16:42 UTC (permalink / raw) To: linux-arm-kernel On 25/09/2012 08:44, Stephen Warren wrote: > On 09/25/2012 05:07 AM, Will Deacon wrote: >> On Mon, Sep 24, 2012 at 11:48:04PM +0100, Markus Mayer wrote: >>> Please let me know if there is a different place this patch should be sent >>> to. Thanks. >> >> You could probably CC devicetree-discuss and also David Gibson for patches >> against dtc. > > And Jon Loeliger. Thanks, guys. -Markus ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 0/1] Fix segfault in DTC 2012-09-24 22:48 [PATCH 0/1] Fix segfault in DTC Markus Mayer 2012-09-24 22:48 ` [PATCH 1/1] " Markus Mayer 2012-09-25 11:07 ` [PATCH 0/1] " Will Deacon @ 2012-09-25 17:58 ` Markus Mayer 2012-09-25 17:58 ` [PATCH 1/1] " Markus Mayer 2 siblings, 1 reply; 22+ messages in thread From: Markus Mayer @ 2012-09-25 17:58 UTC (permalink / raw) To: linux-arm-kernel As suggested on linux-arm-kernel, I am copying devicetree-discuss, as well as David and Jon on this thread. Please let me know your thoughts. Regards, -Markus Markus Mayer (1): Fix segfault in dtc when empty input file is given. scripts/dtc/treesource.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) -- 1.7.5.4 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/1] Fix segfault in DTC 2012-09-25 17:58 ` Markus Mayer @ 2012-09-25 17:58 ` Markus Mayer 2012-09-25 23:30 ` David Gibson 0 siblings, 1 reply; 22+ messages in thread From: Markus Mayer @ 2012-09-25 17:58 UTC (permalink / raw) To: linux-arm-kernel Prior to this change, an empty input file would cause a segfault, because yylloc had never been initialized. There was never any characters for the lexer to match, so YY_USER_ACTION was never executed before the parse error was detected. When the parser printed the error message, it tried to include the name of the file, but the structure holding the file name (yylloc.file, referenced as pos->file) had never been initialized. Without the fix: $ ./dtc /dev/null DTC: dts->dts on file "/dev/null" Segmentation fault (core dumped) $ gdb dtc core Program terminated with signal 11, Segmentation fault. at scripts/dtc/srcpos.c:194 194 fname = pos->file->name; (gdb) bt at scripts/dtc/srcpos.c:194 fmt=0x40d769 "%s", va=0x7fffbf027148) at scripts/dtc/srcpos.c:220 at scripts/dtc/dtc-parser.tab.c:1920 at scripts/dtc/treesource.c:38 at scripts/dtc/dtc.c:203 (gdb) p *pos $1 = {first_line = 0, first_column = 0, last_line = 0, last_column = 0, file = 0x0} With the fix: $ ./dtc /dev/null DTC: dts->dts on file "/dev/null" Error: /dev/null:1.1 syntax error FATAL ERROR: Unable to parse input tree Signed-off-by: Markus Mayer <mmayer@broadcom.com> --- scripts/dtc/treesource.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/scripts/dtc/treesource.c b/scripts/dtc/treesource.c index c09aafa..b461b88 100644 --- a/scripts/dtc/treesource.c +++ b/scripts/dtc/treesource.c @@ -29,11 +29,15 @@ int treesource_error; struct boot_info *dt_from_source(const char *fname) { + extern YYLTYPE yylloc; + the_boot_info = NULL; treesource_error = 0; srcfile_push(fname); yyin = current_srcfile->f; + /* Initialize yylloc->file to avoid segfault on empty input */ + srcpos_update(&yylloc, NULL, 0); if (yyparse() != 0) die("Unable to parse input tree\n"); -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 1/1] Fix segfault in DTC 2012-09-25 17:58 ` [PATCH 1/1] " Markus Mayer @ 2012-09-25 23:30 ` David Gibson 2012-09-25 23:51 ` Markus Mayer 0 siblings, 1 reply; 22+ messages in thread From: David Gibson @ 2012-09-25 23:30 UTC (permalink / raw) To: linux-arm-kernel On Tue, Sep 25, 2012 at 10:58:09AM -0700, Markus Mayer wrote: > Prior to this change, an empty input file would cause a segfault, because > yylloc had never been initialized. There was never any characters for the > lexer to match, so YY_USER_ACTION was never executed before the parse error > was detected. > > When the parser printed the error message, it tried to include the name of > the file, but the structure holding the file name (yylloc.file, referenced > as pos->file) had never been initialized. > > Without the fix: > > $ ./dtc /dev/null > DTC: dts->dts on file "/dev/null" > Segmentation fault (core dumped) > > $ gdb dtc core > Program terminated with signal 11, Segmentation fault. > at scripts/dtc/srcpos.c:194 > 194 fname = pos->file->name; > (gdb) bt > at scripts/dtc/srcpos.c:194 > fmt=0x40d769 "%s", va=0x7fffbf027148) at scripts/dtc/srcpos.c:220 > at scripts/dtc/dtc-parser.tab.c:1920 > at scripts/dtc/treesource.c:38 > at scripts/dtc/dtc.c:203 > (gdb) p *pos > $1 = {first_line = 0, first_column = 0, last_line = 0, last_column = 0, > file = 0x0} Which dtc version did you observe this with? I'm unable to reproduce the SEGV with current git. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/1] Fix segfault in DTC 2012-09-25 23:30 ` David Gibson @ 2012-09-25 23:51 ` Markus Mayer 2012-09-26 0:35 ` David Gibson 0 siblings, 1 reply; 22+ messages in thread From: Markus Mayer @ 2012-09-25 23:51 UTC (permalink / raw) To: linux-arm-kernel On 25/09/2012 16:30, David Gibson wrote: > On Tue, Sep 25, 2012 at 10:58:09AM -0700, Markus Mayer wrote: >> Prior to this change, an empty input file would cause a segfault, because >> yylloc had never been initialized. There was never any characters for the >> lexer to match, so YY_USER_ACTION was never executed before the parse error >> was detected. >> >> When the parser printed the error message, it tried to include the name of >> the file, but the structure holding the file name (yylloc.file, referenced >> as pos->file) had never been initialized. >> >> Without the fix: >> >> $ ./dtc /dev/null >> DTC: dts->dts on file "/dev/null" >> Segmentation fault (core dumped) >> >> $ gdb dtc core >> Program terminated with signal 11, Segmentation fault. >> at scripts/dtc/srcpos.c:194 >> 194 fname = pos->file->name; >> (gdb) bt >> at scripts/dtc/srcpos.c:194 >> fmt=0x40d769 "%s", va=0x7fffbf027148) at scripts/dtc/srcpos.c:220 >> at scripts/dtc/dtc-parser.tab.c:1920 >> at scripts/dtc/treesource.c:38 >> at scripts/dtc/dtc.c:203 >> (gdb) p *pos >> $1 = {first_line = 0, first_column = 0, last_line = 0, last_column = 0, >> file = 0x0} > > Which dtc version did you observe this with? I'm unable to reproduce > the SEGV with current git. That was $ cat version_gen.h #define DTC_VERSION "DTC 1.2.0-g37c0b6a0" from git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git. In other words, the DTC used by the Linux 3.6-rc series. Regards, -Markus ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/1] Fix segfault in DTC 2012-09-25 23:51 ` Markus Mayer @ 2012-09-26 0:35 ` David Gibson 2012-09-26 16:38 ` Markus Mayer 0 siblings, 1 reply; 22+ messages in thread From: David Gibson @ 2012-09-26 0:35 UTC (permalink / raw) To: linux-arm-kernel On Tue, Sep 25, 2012 at 04:51:47PM -0700, Markus Mayer wrote: > On 25/09/2012 16:30, David Gibson wrote: > >On Tue, Sep 25, 2012 at 10:58:09AM -0700, Markus Mayer wrote: > >>Prior to this change, an empty input file would cause a segfault, because > >>yylloc had never been initialized. There was never any characters for the > >>lexer to match, so YY_USER_ACTION was never executed before the parse error > >>was detected. > >> > >>When the parser printed the error message, it tried to include the name of > >>the file, but the structure holding the file name (yylloc.file, referenced > >>as pos->file) had never been initialized. > >> > >>Without the fix: > >> > >>$ ./dtc /dev/null > >>DTC: dts->dts on file "/dev/null" > >>Segmentation fault (core dumped) > >> > >>$ gdb dtc core > >>Program terminated with signal 11, Segmentation fault. > >> at scripts/dtc/srcpos.c:194 > >>194 fname = pos->file->name; > >>(gdb) bt > >> at scripts/dtc/srcpos.c:194 > >> fmt=0x40d769 "%s", va=0x7fffbf027148) at scripts/dtc/srcpos.c:220 > >> at scripts/dtc/dtc-parser.tab.c:1920 > >> at scripts/dtc/treesource.c:38 > >> at scripts/dtc/dtc.c:203 > >>(gdb) p *pos > >>$1 = {first_line = 0, first_column = 0, last_line = 0, last_column = 0, > >> file = 0x0} > > > >Which dtc version did you observe this with? I'm unable to reproduce > >the SEGV with current git. > > That was > > $ cat version_gen.h > #define DTC_VERSION "DTC 1.2.0-g37c0b6a0" > > from > git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git. In > other words, the DTC used by the Linux 3.6-rc series. Ah, ok. Patches to dtc should be made against the upstream version at git://git.jdl.com/software/dtc.git. In this case the bug was already fixed there, in commit a6e6c60e3a97a6b3a033cd052bb3740fd53cbf4c. I think we're overdue to update the kernel copy of dtc from upstream. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/1] Fix segfault in DTC 2012-09-26 0:35 ` David Gibson @ 2012-09-26 16:38 ` Markus Mayer 2012-09-28 17:09 ` Stephen Warren 0 siblings, 1 reply; 22+ messages in thread From: Markus Mayer @ 2012-09-26 16:38 UTC (permalink / raw) To: linux-arm-kernel On 25/09/2012 17:35, David Gibson wrote: > On Tue, Sep 25, 2012 at 04:51:47PM -0700, Markus Mayer wrote: >> On 25/09/2012 16:30, David Gibson wrote: >>> >>> Which dtc version did you observe this with? I'm unable to reproduce >>> the SEGV with current git. >> >> That was >> >> $ cat version_gen.h >> #define DTC_VERSION "DTC 1.2.0-g37c0b6a0" >> >> from >> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git. In >> other words, the DTC used by the Linux 3.6-rc series. > > Ah, ok. Patches to dtc should be made against the upstream version at > git://git.jdl.com/software/dtc.git. In this case the bug was already > fixed there, in commit a6e6c60e3a97a6b3a033cd052bb3740fd53cbf4c. Ah yes. That makes sense. Thanks for pointing out the commit that fixes the issue. I just checked it out. > I think we're overdue to update the kernel copy of dtc from upstream. Yeah, seems like the kernel DTC is quite old. Regards, -Markus ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/1] Fix segfault in DTC 2012-09-26 16:38 ` Markus Mayer @ 2012-09-28 17:09 ` Stephen Warren 2012-09-28 18:53 ` Jon Loeliger 0 siblings, 1 reply; 22+ messages in thread From: Stephen Warren @ 2012-09-28 17:09 UTC (permalink / raw) To: linux-arm-kernel On 09/26/2012 10:38 AM, Markus Mayer wrote: > On 25/09/2012 17:35, David Gibson wrote: >> On Tue, Sep 25, 2012 at 04:51:47PM -0700, Markus Mayer wrote: >>> On 25/09/2012 16:30, David Gibson wrote: >>>> >>>> Which dtc version did you observe this with? I'm unable to reproduce >>>> the SEGV with current git. >>> >>> That was >>> >>> $ cat version_gen.h >>> #define DTC_VERSION "DTC 1.2.0-g37c0b6a0" >>> >>> from >>> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git. In >>> other words, the DTC used by the Linux 3.6-rc series. >> >> Ah, ok. Patches to dtc should be made against the upstream version at >> git://git.jdl.com/software/dtc.git. In this case the bug was already >> fixed there, in commit a6e6c60e3a97a6b3a033cd052bb3740fd53cbf4c. > > Ah yes. That makes sense. Thanks for pointing out the commit that fixes > the issue. I just checked it out. > >> I think we're overdue to update the kernel copy of dtc from upstream. > > Yeah, seems like the kernel DTC is quite old. FYI, I'm working on a patch to the kernel to bring in the latest dtc. I've run a regression test vs. the old dtc in the kernel and found that some of the PowerPC .dts files don't compile with the new dtc (but did with the old), all due to non-existent labels/paths being referenced. I'll try and track down whether this is a regression in dtc, or simply buggy .dts files that weren't noticed before. Everything non-PowerPC works fine, and most PowerPC files work fine. Where compilation succeeds, the md5sum of the .dtb is identical in every case. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/1] Fix segfault in DTC 2012-09-28 17:09 ` Stephen Warren @ 2012-09-28 18:53 ` Jon Loeliger 2012-09-28 19:05 ` Stephen Warren 0 siblings, 1 reply; 22+ messages in thread From: Jon Loeliger @ 2012-09-28 18:53 UTC (permalink / raw) To: linux-arm-kernel > > > > Yeah, seems like the kernel DTC is quite old. > > FYI, I'm working on a patch to the kernel to bring in the latest dtc. Awesome. Thank you. > I've run a regression test vs. the old dtc in the kernel ... Which is the icky step. Again, thank you. > ... and found that > some of the PowerPC .dts files don't compile with the new dtc (but did > with the old), all due to non-existent labels/paths being referenced. > I'll try and track down whether this is a regression in dtc, or simply > buggy .dts files that weren't noticed before. I think you should just smack the PowerPC guys. :-) > Everything non-PowerPC works fine, and most PowerPC files work fine. > Where compilation succeeds, the md5sum of the .dtb is identical in every > case. Cool. Thanks, jdl ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/1] Fix segfault in DTC 2012-09-28 18:53 ` Jon Loeliger @ 2012-09-28 19:05 ` Stephen Warren 2012-09-28 20:32 ` Jon Loeliger 2012-09-29 23:53 ` David Gibson 0 siblings, 2 replies; 22+ messages in thread From: Stephen Warren @ 2012-09-28 19:05 UTC (permalink / raw) To: linux-arm-kernel On 09/28/2012 12:53 PM, Jon Loeliger wrote: >>> >>> Yeah, seems like the kernel DTC is quite old. >> >> FYI, I'm working on a patch to the kernel to bring in the latest dtc. > > Awesome. Thank you. > >> I've run a regression test vs. the old dtc in the kernel ... > > Which is the icky step. Again, thank you. > >> ... and found that >> some of the PowerPC .dts files don't compile with the new dtc (but did >> with the old), all due to non-existent labels/paths being referenced. >> I'll try and track down whether this is a regression in dtc, or simply >> buggy .dts files that weren't noticed before. > > I think you should just smack the PowerPC guys. :-) For the record in this thread, it was a regression I introduced into dtc - the patch I just sent was for this. >> Everything non-PowerPC works fine, and most PowerPC files work fine. >> Where compilation succeeds, the md5sum of the .dtb is identical in every >> case. ... and with that patch, every .dts in the kernel compiles to the same md5sum before/after. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/1] Fix segfault in DTC 2012-09-28 19:05 ` Stephen Warren @ 2012-09-28 20:32 ` Jon Loeliger 2012-09-29 23:53 ` David Gibson 1 sibling, 0 replies; 22+ messages in thread From: Jon Loeliger @ 2012-09-28 20:32 UTC (permalink / raw) To: linux-arm-kernel > > > > I think you should just smack the PowerPC guys. :-) > > For the record in this thread, it was a regression I introduced into dtc > - the patch I just sent was for this. Ah, ok. I'll apply the DTC patch, but I think you should still smack the PowerPC guys on general principle. :-) jdl ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/1] Fix segfault in DTC 2012-09-28 19:05 ` Stephen Warren 2012-09-28 20:32 ` Jon Loeliger @ 2012-09-29 23:53 ` David Gibson 2012-10-01 5:34 ` Stephen Warren 1 sibling, 1 reply; 22+ messages in thread From: David Gibson @ 2012-09-29 23:53 UTC (permalink / raw) To: linux-arm-kernel On Fri, Sep 28, 2012 at 01:05:33PM -0600, Stephen Warren wrote: > On 09/28/2012 12:53 PM, Jon Loeliger wrote: > >>> > >>> Yeah, seems like the kernel DTC is quite old. > >> > >> FYI, I'm working on a patch to the kernel to bring in the latest dtc. > > > > Awesome. Thank you. > > > >> I've run a regression test vs. the old dtc in the kernel ... > > > > Which is the icky step. Again, thank you. > > > >> ... and found that > >> some of the PowerPC .dts files don't compile with the new dtc (but did > >> with the old), all due to non-existent labels/paths being referenced. > >> I'll try and track down whether this is a regression in dtc, or simply > >> buggy .dts files that weren't noticed before. > > > > I think you should just smack the PowerPC guys. :-) > > For the record in this thread, it was a regression I introduced into dtc > - the patch I just sent was for this. I would be nice to add a testcase for this regression into dtc. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/1] Fix segfault in DTC 2012-09-29 23:53 ` David Gibson @ 2012-10-01 5:34 ` Stephen Warren 2012-10-01 6:46 ` David Gibson 0 siblings, 1 reply; 22+ messages in thread From: Stephen Warren @ 2012-10-01 5:34 UTC (permalink / raw) To: linux-arm-kernel On 09/29/2012 05:53 PM, David Gibson wrote: > On Fri, Sep 28, 2012 at 01:05:33PM -0600, Stephen Warren wrote: >> On 09/28/2012 12:53 PM, Jon Loeliger wrote: >>>>> >>>>> Yeah, seems like the kernel DTC is quite old. >>>> >>>> FYI, I'm working on a patch to the kernel to bring in the latest dtc. >>> >>> Awesome. Thank you. >>> >>>> I've run a regression test vs. the old dtc in the kernel ... >>> >>> Which is the icky step. Again, thank you. >>> >>>> ... and found that >>>> some of the PowerPC .dts files don't compile with the new dtc (but did >>>> with the old), all due to non-existent labels/paths being referenced. >>>> I'll try and track down whether this is a regression in dtc, or simply >>>> buggy .dts files that weren't noticed before. >>> >>> I think you should just smack the PowerPC guys. :-) >> >> For the record in this thread, it was a regression I introduced into dtc >> - the patch I just sent was for this. > > I would be nice to add a testcase for this regression into dtc. The issue here was caused by uninitialized memory, so it would, I think, be basically impossible to create a test-case that would be guaranteed to fail because of this; it'd depend on the internal details of the malloc library and how/when it re-used previously free()d memory blocks. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/1] Fix segfault in DTC 2012-10-01 5:34 ` Stephen Warren @ 2012-10-01 6:46 ` David Gibson 2012-10-01 16:41 ` Stephen Warren 0 siblings, 1 reply; 22+ messages in thread From: David Gibson @ 2012-10-01 6:46 UTC (permalink / raw) To: linux-arm-kernel On Sun, Sep 30, 2012 at 11:34:50PM -0600, Stephen Warren wrote: > On 09/29/2012 05:53 PM, David Gibson wrote: > > On Fri, Sep 28, 2012 at 01:05:33PM -0600, Stephen Warren wrote: > >> On 09/28/2012 12:53 PM, Jon Loeliger wrote: > >>>>> > >>>>> Yeah, seems like the kernel DTC is quite old. > >>>> > >>>> FYI, I'm working on a patch to the kernel to bring in the latest dtc. > >>> > >>> Awesome. Thank you. > >>> > >>>> I've run a regression test vs. the old dtc in the kernel ... > >>> > >>> Which is the icky step. Again, thank you. > >>> > >>>> ... and found that > >>>> some of the PowerPC .dts files don't compile with the new dtc (but did > >>>> with the old), all due to non-existent labels/paths being referenced. > >>>> I'll try and track down whether this is a regression in dtc, or simply > >>>> buggy .dts files that weren't noticed before. > >>> > >>> I think you should just smack the PowerPC guys. :-) > >> > >> For the record in this thread, it was a regression I introduced into dtc > >> - the patch I just sent was for this. > > > > I would be nice to add a testcase for this regression into dtc. > > The issue here was caused by uninitialized memory, so it would, I think, > be basically impossible to create a test-case that would be guaranteed > to fail because of this; it'd depend on the internal details of the > malloc library and how/when it re-used previously free()d memory blocks. It doesn't have to be guaranteed to fail to be useful. Plus, we already have the infrastructure to run the tests under valgrind, which would catch it. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/1] Fix segfault in DTC 2012-10-01 6:46 ` David Gibson @ 2012-10-01 16:41 ` Stephen Warren 2012-10-02 0:08 ` David Gibson 0 siblings, 1 reply; 22+ messages in thread From: Stephen Warren @ 2012-10-01 16:41 UTC (permalink / raw) To: linux-arm-kernel On 10/01/2012 12:46 AM, David Gibson wrote: > On Sun, Sep 30, 2012 at 11:34:50PM -0600, Stephen Warren wrote: >> On 09/29/2012 05:53 PM, David Gibson wrote: >>> On Fri, Sep 28, 2012 at 01:05:33PM -0600, Stephen Warren wrote: >>>> On 09/28/2012 12:53 PM, Jon Loeliger wrote: >>>>>>> >>>>>>> Yeah, seems like the kernel DTC is quite old. >>>>>> >>>>>> FYI, I'm working on a patch to the kernel to bring in the latest dtc. >>>>> >>>>> Awesome. Thank you. >>>>> >>>>>> I've run a regression test vs. the old dtc in the kernel ... >>>>> >>>>> Which is the icky step. Again, thank you. >>>>> >>>>>> ... and found that >>>>>> some of the PowerPC .dts files don't compile with the new dtc (but did >>>>>> with the old), all due to non-existent labels/paths being referenced. >>>>>> I'll try and track down whether this is a regression in dtc, or simply >>>>>> buggy .dts files that weren't noticed before. >>>>> >>>>> I think you should just smack the PowerPC guys. :-) >>>> >>>> For the record in this thread, it was a regression I introduced into dtc >>>> - the patch I just sent was for this. >>> >>> I would be nice to add a testcase for this regression into dtc. >> >> The issue here was caused by uninitialized memory, so it would, I think, >> be basically impossible to create a test-case that would be guaranteed >> to fail because of this; it'd depend on the internal details of the >> malloc library and how/when it re-used previously free()d memory blocks. > > It doesn't have to be guaranteed to fail to be useful. Plus, we > already have the infrastructure to run the tests under valgrind, which > would catch it. I certainly disagree here; the absolute worst kind of test is one which gives different results each time it's run, or statically gives different results to different people. People will either ignore the test because it's flaky, or it'll end up blaming the wrong person due to some entirely unrelated and correct change just happening to tickle the test. If we were to force any such new test to always run under valgrind, then hopefully the test would always fail (assuming the test harness triggers failure if valgrind finds problems). That might be reasonable. However, if we do this, then I think instead we should just always force the entire existing test suite to run under valgrind; no new test should be needed for valgrind to detect this failure (since there was a read-before-write problem any time a label was used in the existing code, it just didn't always cause a user-visible issue). Another alternative might be to LD_PRELOAD a test malloc library with a known allocation pattern. That would allow a new test for this case to always fail. However, the allocation pattern of this test malloc library would have to be carefully tied to the allocation pattern of dtc itself when processing the new test's .dts file, and that could easily and legitimately change due to unrelated implementation details of dtc changing, thus making the maintenance of the test malloc library painful. So, I don't think this is the way to go. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/1] Fix segfault in DTC 2012-10-01 16:41 ` Stephen Warren @ 2012-10-02 0:08 ` David Gibson 2012-10-03 21:33 ` Stephen Warren 0 siblings, 1 reply; 22+ messages in thread From: David Gibson @ 2012-10-02 0:08 UTC (permalink / raw) To: linux-arm-kernel On Mon, Oct 01, 2012 at 10:41:09AM -0600, Stephen Warren wrote: > On 10/01/2012 12:46 AM, David Gibson wrote: > > On Sun, Sep 30, 2012 at 11:34:50PM -0600, Stephen Warren wrote: > >> On 09/29/2012 05:53 PM, David Gibson wrote: > >>> On Fri, Sep 28, 2012 at 01:05:33PM -0600, Stephen Warren wrote: > >>>> On 09/28/2012 12:53 PM, Jon Loeliger wrote: > >>>>>>> > >>>>>>> Yeah, seems like the kernel DTC is quite old. > >>>>>> > >>>>>> FYI, I'm working on a patch to the kernel to bring in the latest dtc. > >>>>> > >>>>> Awesome. Thank you. > >>>>> > >>>>>> I've run a regression test vs. the old dtc in the kernel ... > >>>>> > >>>>> Which is the icky step. Again, thank you. > >>>>> > >>>>>> ... and found that > >>>>>> some of the PowerPC .dts files don't compile with the new dtc (but did > >>>>>> with the old), all due to non-existent labels/paths being referenced. > >>>>>> I'll try and track down whether this is a regression in dtc, or simply > >>>>>> buggy .dts files that weren't noticed before. > >>>>> > >>>>> I think you should just smack the PowerPC guys. :-) > >>>> > >>>> For the record in this thread, it was a regression I introduced into dtc > >>>> - the patch I just sent was for this. > >>> > >>> I would be nice to add a testcase for this regression into dtc. > >> > >> The issue here was caused by uninitialized memory, so it would, I think, > >> be basically impossible to create a test-case that would be guaranteed > >> to fail because of this; it'd depend on the internal details of the > >> malloc library and how/when it re-used previously free()d memory blocks. > > > > It doesn't have to be guaranteed to fail to be useful. Plus, we > > already have the infrastructure to run the tests under valgrind, which > > would catch it. > > I certainly disagree here; the absolute worst kind of test is one which > gives different results each time it's run, or statically gives > different results to different people. People will either ignore the > test because it's flaky, or it'll end up blaming the wrong person due to > some entirely unrelated and correct change just happening to tickle the > test. I'd agree 100% if the test could give false failures. But in this case it can only give false passes. If the test fails there is a bug *somewhere*, even if it's not actually in whatever changed last. The test framework actually has a "PASS (inconclusive)" result for exactly this sort of case. > If we were to force any such new test to always run under valgrind, then > hopefully the test would always fail (assuming the test harness triggers > failure if valgrind finds problems). Aside: it's supposed to; if it doesn't, that's a bug. You can try it easily enough with "make checkm". > That might be reasonable. However, > if we do this, then I think instead we should just always force the > entire existing test suite to run under valgrind; no new test should be > needed for valgrind to detect this failure (since there was a > read-before-write problem any time a label was used in the existing > code, it just didn't always cause a user-visible issue). Hrm. I don't want to run the whole testsuite under valgrind by default for a couple of reasons. First, it's much, much slower, and second you need to have valgrind installed. Both these would discourage people from running the testsuite so frequently, and even without valgrind it's very, very useful. I guess we could make the new one a "valgrind only" test, that's we only bother to execute when the valgrind options are enabled. > Another alternative might be to LD_PRELOAD a test malloc library with a > known allocation pattern. That would allow a new test for this case to > always fail. However, the allocation pattern of this test malloc library > would have to be carefully tied to the allocation pattern of dtc itself > when processing the new test's .dts file, and that could easily and > legitimately change due to unrelated implementation details of dtc > changing, thus making the maintenance of the test malloc library > painful. So, I don't think this is the way to go. Hrm. A malloc() with a "known buggy" allocation pattern would be impractical, I agree. But it occurred to me that a much simpler malloc() wrapper which filled each allocated block with garbage (non-zero) data before returning it would be sufficient in this case. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/1] Fix segfault in DTC 2012-10-02 0:08 ` David Gibson @ 2012-10-03 21:33 ` Stephen Warren 2012-10-04 4:49 ` David Gibson 0 siblings, 1 reply; 22+ messages in thread From: Stephen Warren @ 2012-10-03 21:33 UTC (permalink / raw) To: linux-arm-kernel On 10/01/2012 06:08 PM, David Gibson wrote: > On Mon, Oct 01, 2012 at 10:41:09AM -0600, Stephen Warren wrote: >> On 10/01/2012 12:46 AM, David Gibson wrote: >>> On Sun, Sep 30, 2012 at 11:34:50PM -0600, Stephen Warren wrote: >>>> On 09/29/2012 05:53 PM, David Gibson wrote: >>>>> On Fri, Sep 28, 2012 at 01:05:33PM -0600, Stephen Warren wrote: >>>>>> On 09/28/2012 12:53 PM, Jon Loeliger wrote: >>>>>>>>> >>>>>>>>> Yeah, seems like the kernel DTC is quite old. >>>>>>>> >>>>>>>> FYI, I'm working on a patch to the kernel to bring in the latest dtc. >>>>>>> >>>>>>> Awesome. Thank you. >>>>>>> >>>>>>>> I've run a regression test vs. the old dtc in the kernel ... >>>>>>> >>>>>>> Which is the icky step. Again, thank you. >>>>>>> >>>>>>>> ... and found that >>>>>>>> some of the PowerPC .dts files don't compile with the new dtc (but did >>>>>>>> with the old), all due to non-existent labels/paths being referenced. >>>>>>>> I'll try and track down whether this is a regression in dtc, or simply >>>>>>>> buggy .dts files that weren't noticed before. >>>>>>> >>>>>>> I think you should just smack the PowerPC guys. :-) >>>>>> >>>>>> For the record in this thread, it was a regression I introduced into dtc >>>>>> - the patch I just sent was for this. >>>>> >>>>> I would be nice to add a testcase for this regression into dtc. >>>> >>>> The issue here was caused by uninitialized memory, so it would, I think, >>>> be basically impossible to create a test-case that would be guaranteed >>>> to fail because of this; it'd depend on the internal details of the >>>> malloc library and how/when it re-used previously free()d memory blocks. >>> >>> It doesn't have to be guaranteed to fail to be useful. Plus, we >>> already have the infrastructure to run the tests under valgrind, which >>> would catch it. >> >> I certainly disagree here; the absolute worst kind of test is one which >> gives different results each time it's run, or statically gives >> different results to different people. People will either ignore the >> test because it's flaky, or it'll end up blaming the wrong person due to >> some entirely unrelated and correct change just happening to tickle the >> test. > > I'd agree 100% if the test could give false failures. But in this > case it can only give false passes. That is true. I still dislike flaky tests irrespective of false-{negative,positive} though. > If the test fails there is a bug > *somewhere*, even if it's not actually in whatever changed last. The > test framework actually has a "PASS (inconclusive)" result for exactly > this sort of case. > >> If we were to force any such new test to always run under valgrind, then >> hopefully the test would always fail (assuming the test harness triggers >> failure if valgrind finds problems). > > Aside: it's supposed to; if it doesn't, that's a bug. You can try it > easily enough with "make checkm". Ah, I didn't know about that make target (or even "make check"; I'd always run run_tests.sh manually). Incidentally, before commit 317a5d9 "dtc: zero out new label objects" the following two tests fail under make checkm: dtc -I dts -O dtb -o multilabel.test.dtb multilabel.dts: FAIL Returned error code 126 dtc -I dts -O dtb -o multilabel_merge.test.dtb multilabel_merge.dts: FAIL Returned error code 126 However, they pass at/after that commit. Equally, those failures were introduced with commit 45013d8 "dtc: Add ability to delete nodes and properties", which is exactly what I'd expect given the fix was a fix for that commit. So, it seems like we already have tests that catch this problem. Do we need to do anything given that? I am slightly surprised that the problem didn't cause all tests to fail make checkm though (I'd expect any usage of a label to trigger the problem); I'll have to think about why some more... ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/1] Fix segfault in DTC 2012-10-03 21:33 ` Stephen Warren @ 2012-10-04 4:49 ` David Gibson 0 siblings, 0 replies; 22+ messages in thread From: David Gibson @ 2012-10-04 4:49 UTC (permalink / raw) To: linux-arm-kernel On Wed, Oct 03, 2012 at 03:33:30PM -0600, Stephen Warren wrote: > On 10/01/2012 06:08 PM, David Gibson wrote: > > On Mon, Oct 01, 2012 at 10:41:09AM -0600, Stephen Warren wrote: > >> On 10/01/2012 12:46 AM, David Gibson wrote: > >>> On Sun, Sep 30, 2012 at 11:34:50PM -0600, Stephen Warren wrote: > >>>> On 09/29/2012 05:53 PM, David Gibson wrote: > >>>>> On Fri, Sep 28, 2012 at 01:05:33PM -0600, Stephen Warren wrote: > >>>>>> On 09/28/2012 12:53 PM, Jon Loeliger wrote: > >>>>>>>>> > >>>>>>>>> Yeah, seems like the kernel DTC is quite old. > >>>>>>>> > >>>>>>>> FYI, I'm working on a patch to the kernel to bring in the latest dtc. > >>>>>>> > >>>>>>> Awesome. Thank you. > >>>>>>> > >>>>>>>> I've run a regression test vs. the old dtc in the kernel ... > >>>>>>> > >>>>>>> Which is the icky step. Again, thank you. > >>>>>>> > >>>>>>>> ... and found that > >>>>>>>> some of the PowerPC .dts files don't compile with the new dtc (but did > >>>>>>>> with the old), all due to non-existent labels/paths being referenced. > >>>>>>>> I'll try and track down whether this is a regression in dtc, or simply > >>>>>>>> buggy .dts files that weren't noticed before. > >>>>>>> > >>>>>>> I think you should just smack the PowerPC guys. :-) > >>>>>> > >>>>>> For the record in this thread, it was a regression I introduced into dtc > >>>>>> - the patch I just sent was for this. > >>>>> > >>>>> I would be nice to add a testcase for this regression into dtc. > >>>> > >>>> The issue here was caused by uninitialized memory, so it would, I think, > >>>> be basically impossible to create a test-case that would be guaranteed > >>>> to fail because of this; it'd depend on the internal details of the > >>>> malloc library and how/when it re-used previously free()d memory blocks. > >>> > >>> It doesn't have to be guaranteed to fail to be useful. Plus, we > >>> already have the infrastructure to run the tests under valgrind, which > >>> would catch it. > >> > >> I certainly disagree here; the absolute worst kind of test is one which > >> gives different results each time it's run, or statically gives > >> different results to different people. People will either ignore the > >> test because it's flaky, or it'll end up blaming the wrong person due to > >> some entirely unrelated and correct change just happening to tickle the > >> test. > > > > I'd agree 100% if the test could give false failures. But in this > > case it can only give false passes. > > That is true. I still dislike flaky tests irrespective of > false-{negative,positive} though. I don't love it, but I think a test which can generate false-negatives is better than no test. > > If the test fails there is a bug > > *somewhere*, even if it's not actually in whatever changed last. The > > test framework actually has a "PASS (inconclusive)" result for exactly > > this sort of case. > > > >> If we were to force any such new test to always run under valgrind, then > >> hopefully the test would always fail (assuming the test harness triggers > >> failure if valgrind finds problems). > > > > Aside: it's supposed to; if it doesn't, that's a bug. You can try it > > easily enough with "make checkm". > > Ah, I didn't know about that make target (or even "make check"; I'd > always run run_tests.sh manually). > > Incidentally, before commit 317a5d9 "dtc: zero out new label objects" > the following two tests fail under make checkm: > > dtc -I dts -O dtb -o multilabel.test.dtb multilabel.dts: FAIL > Returned error code 126 > > dtc -I dts -O dtb -o multilabel_merge.test.dtb multilabel_merge.dts: > FAIL Returned error code 126 > > However, they pass at/after that commit. Ah, interesting. Goes to show that neither Jon nor I runs the valgrind check as often as we probably should. > Equally, those failures were introduced with commit 45013d8 "dtc: Add > ability to delete nodes and properties", which is exactly what I'd > expect given the fix was a fix for that commit. > > So, it seems like we already have tests that catch this problem. Do we > need to do anything given that? Hm, I guess not. > I am slightly surprised that the problem didn't cause all tests to fail > make checkm though (I'd expect any usage of a label to trigger the > problem); I'll have to think about why some more... -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2012-10-04 4:49 UTC | newest] Thread overview: 22+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-09-24 22:48 [PATCH 0/1] Fix segfault in DTC Markus Mayer 2012-09-24 22:48 ` [PATCH 1/1] " Markus Mayer 2012-09-25 11:07 ` [PATCH 0/1] " Will Deacon 2012-09-25 15:44 ` Stephen Warren 2012-09-25 16:42 ` Markus Mayer 2012-09-25 17:58 ` Markus Mayer 2012-09-25 17:58 ` [PATCH 1/1] " Markus Mayer 2012-09-25 23:30 ` David Gibson 2012-09-25 23:51 ` Markus Mayer 2012-09-26 0:35 ` David Gibson 2012-09-26 16:38 ` Markus Mayer 2012-09-28 17:09 ` Stephen Warren 2012-09-28 18:53 ` Jon Loeliger 2012-09-28 19:05 ` Stephen Warren 2012-09-28 20:32 ` Jon Loeliger 2012-09-29 23:53 ` David Gibson 2012-10-01 5:34 ` Stephen Warren 2012-10-01 6:46 ` David Gibson 2012-10-01 16:41 ` Stephen Warren 2012-10-02 0:08 ` David Gibson 2012-10-03 21:33 ` Stephen Warren 2012-10-04 4:49 ` David Gibson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).