Fabian,
I totally agree with defensive code to prevent segfault and other
"unnecessary errors".
However, this checkin makes the follwoing tests fail by producing a
different error than before, i.e., they now fail with the newly added
parseError --- and most probably correctly, so, because the tests (try to)
trigger some other error but indeed declare command that reference a
non-existing address to do so.
monetdb5/mal/Tests/tst102.mal
monetdb5/mal/Tests/tst104.mal
monetdb5/mal/Tests/tst105.mal
monetdb5/mal/Tests/tst105a.mal
monetdb5/mal/Tests/tst106.mal
monetdb5/mal/Tests/tst115.mal
monetdb5/mal/Tests/tst150.mal
monetdb5/mal/Tests/dynamicload.mal
I'm not sure, whether we should simply approve the new output, as then the
tests would no longer test what they originally intended to test.
Probably, we should design the tests properly, but I could not come up with
a quick solution.
In case your newly added parseError defense is a bit too strict, i.e.,
required only to prevent calling malAtomProperty() with curInstr->fcn ==
NULL, the following patch might be considered:
========
diff -r a4f844538f0c monetdb5/mal/mal_parser.mx
--- a/monetdb5/mal/mal_parser.mx Sun Mar 06 12:59:51 2011 +0100
+++ b/monetdb5/mal/mal_parser.mx Sun Mar 06 14:01:48 2011 +0100
@@ -1330,11 +1330,12 @@
setModuleScope(curInstr,
findModule(cntxt->nspace,
putName(modnme,strlen(modnme))) );
curInstr->fcn = getAddress(cntxt->srcFile, modnme, nme,TRUE);
- if (curInstr->fcn == NULL)
- return (MalBlkPtr) parseError(cntxt, "<address> not
found\n");
curBlk->binding = nme;
- if( cntxt->nspace->isAtomModule)
+ if( cntxt->nspace->isAtomModule) {
+ if (curInstr->fcn == NULL)
+ return (MalBlkPtr) xparseError(cntxt,
"<address> not found\n");
malAtomProperty(curBlk, curInstr);
+ }
skipSpace(cntxt);
} else {
return (MalBlkPtr) parseError(cntxt,"'address' expected\n");
========
This would assume, though, that curInstr->fcn == NULL in other cases is
properly checked and handled elsewhere ...
Any opinions?
Stefan
On Sat, Mar 05, 2011 at 09:44:10PM +0100, Fabian Groffen wrote:
> Changeset: 8fa1212bfe7c for MonetDB
> URL: http://dev.monetdb.org/hg/MonetDB?cmd=changeset;node=8fa1212bfe7c
> Modified Files:
> monetdb5/mal/mal_parser.mx
> Branch: Mar2011
> Log Message:
>
> parseCommandPattern: return an error when an address wasn't found
>
> On Darwin some more modules than on Linux are missing, which is probably
> the cause why this went unnoticed. If no address was found, fcn will be
> NULL, which in turn will lead to a dereference in malAtomPropery. So
> return an error instead.
>
>
> diffs (12 lines):
>
> diff --git a/monetdb5/mal/mal_parser.mx b/monetdb5/mal/mal_parser.mx
> --- a/monetdb5/mal/mal_parser.mx
> +++ b/monetdb5/mal/mal_parser.mx
> @@ -1330,6 +1330,8 @@
> setModuleScope(curInstr,
> findModule(cntxt->nspace,
> putName(modnme,strlen(modnme))) );
> curInstr->fcn = getAddress(cntxt->srcFile, modnme, nme,TRUE);
> + if (curInstr->fcn == NULL)
> + return (MalBlkPtr) parseError(cntxt, "<address> not
> found\n");
> curBlk->binding = nme;
> if( cntxt->nspace->isAtomModule)
> malAtomProperty(curBlk, curInstr);
> _______________________________________________
> Checkin-list mailing list
> [email protected]
> http://mail.monetdb.org/mailman/listinfo/checkin-list
--
| Stefan.Manegold @ CWI.nl | DB Architectures (INS1) |
| http://CWI.nl/~manegold/ | Science Park 123 (L321) |
| Tel.: +31 (0)20 592-4212 | 1098 XG Amsterdam (NL) |
_______________________________________________
Checkin-list mailing list
[email protected]
http://mail.monetdb.org/mailman/listinfo/checkin-list
