| Message ID | 1348595889-6495-2-git-send-email-mmayer@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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
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.
> > > > 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
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.
> > > > 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
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.
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.
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.
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.
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.
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...
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...
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");
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(-)