Board index FlightGear Development

Nasal bugfixes - for 2.12?

FlightGear is opensource, so you can be the developer. In the need for help on anything? We are here to help you.
Forum rules
Core development is discussed on the official FlightGear-Devel development mailing list.

Bugs can be reported in the bug tracker.

Nasal bugfixes - for 2.12?

Postby Philosopher » Sun Jul 21, 2013 2:50 pm

Would it be possible to include my two Nasal bugfixes in the next release? I guarantee they are 100% safe (+20% because it was deficient before ;)) and I believe them to be of semi-high priority because they're the most annoying thing in Nasal (besides the inability to iterate through hashes, but that's a "new feature"). Please? It's barely any code :).

P.S. Sorry for breaking all protocol wrt to release plan "use the bug tracker":(.
Philosopher
 
Posts: 1593
Joined: Sun Aug 12, 2012 7:29 pm

Re: Nasal bugfixes - for 2.12?

Postby Philosopher » Sun Jul 21, 2013 4:22 pm

Code: Select all
static int defArg(struct Parser* p, struct Token* t)
{
    if(t->type == TOK_LPAR) {
        if (LEFT(t) != RIGHT(t))
            naParseError(p, "cannot have default argument as function call", t->line);
        return defArg(p, RIGHT(t));
    }
    if(t->type == TOK_MINUS && RIGHT(t) &&
       RIGHT(t)->type == TOK_LITERAL && !RIGHT(t)->str)
    {
        /* default arguments are constants, but "-1" parses as two
         * tokens, so we have to subset the expression generator for that
         * case */
        RIGHT(t)->num *= -1;
        return defArg(p, RIGHT(t));
    }
    return findConstantIndex(p, t);
}

static void genAssign(struct Parser* p, struct Token* t)
///.....
        if(LEFT(rv) == RIGHT(rv) && rv->type == TOK_LPAR) {
            if(len != parListLen(rv))
                naParseError(p, "bad assignment count", rv->line);
            genCommaList(p, LEFT(rv));
        } else {
            genExpr(p, rv);
            emitImmediate(p, OP_UNPACK, len);
        }
///......
}
Philosopher
 
Posts: 1593
Joined: Sun Aug 12, 2012 7:29 pm

Re: Nasal bugfixes - for 2.12?

Postby zakalawe » Mon Jul 22, 2013 1:36 pm

I'm happy to consider bug-fixes for 2.12, but I need a separate patch for each fix, and a test case (Nasal snippet) which fails without the patch, and passes with it. I also need a risk-confidence assessment to convince me it's not going to suddenly break lots of code in the wild :)

Patches here is fine, ideally in unified diff format. A merge request is also fine, but more hassle so if they're small don't worry about it.

The most important thing is making the change small and clear enough, that I can convince myself it's safe, since if I merge it and the universe explodes, I'll get shouted at, a lot.

I can /guess/ from the code above, one issue is with parsing of negative integer default arguments, but without a bit more info I can't proceed. A bug report (or two, actually) would be easier to deal with but apparently some people don't agree :)
zakalawe
 
Posts: 1259
Joined: Sat Jul 19, 2008 5:48 pm
Location: Edinburgh, Scotland
Callsign: G-ZKLW
Version: next
OS: Mac

Re: Nasal bugfixes - for 2.12?

Postby Philosopher » Mon Jul 22, 2013 1:54 pm

Don't worry, you won't get yelled at ;). I guarantee it 160%.

Anyways, sorry for not saying this above, but these are actually fixes for bugs 585 and 737 (palindromic issues!). This just adds RIGHT(t)==LEFT(t) checks, as Andy had fixed the negative issue a long time ago (I just copied the whole function for short). Diff will be coming in a little bit.

Snippets (in order of above functions):
Code: Select all
func(f=h()) {}

Code: Select all
var (a,b) = props.globals.getNode("sim").getChildren("view")
# Previously it required .getChildren("view")[:]
Philosopher
 
Posts: 1593
Joined: Sun Aug 12, 2012 7:29 pm

Re: Nasal bugfixes - for 2.12?

Postby Philosopher » Mon Jul 22, 2013 2:42 pm

Code: Select all
diff --git a/src/codegen.c b/src/codegen.c
index 059c114..f309107 100644
--- a/src/codegen.c
+++ b/src/codegen.c
@@ -147,7 +147,11 @@ static void genEqOp(int op, struct Parser* p, struct Token* t)
 
 static int defArg(struct Parser* p, struct Token* t)
 {
-    if(t->type == TOK_LPAR) return defArg(p, RIGHT(t));
+    if(t->type == TOK_LPAR) {
+        if(LEFT(t) != RIGHT(t))
+            naParseError(p, "cannot have default argument as function call", t->line);
+        return defArg(p, RIGHT(t));
+    }
     if(t->type == TOK_MINUS && RIGHT(t) &&
        RIGHT(t)->type == TOK_LITERAL && !RIGHT(t)->str)
     {
@@ -542,7 +546,7 @@ static void genAssign(struct Parser* p, struct Token* t)
     if(parListLen(lv) || (lv->type == TOK_VAR && parListLen(RIGHT(lv)))) {
         if(lv->type == TOK_VAR) { lv = RIGHT(lv); var = 1; }
         len = parListLen(lv);
-        if(rv->type == TOK_LPAR) {
+        if(LEFT(rv) == RIGHT(rv) && rv->type == TOK_LPAR) {
             if(len != parListLen(rv))
                 naParseError(p, "bad assignment count", rv->line);
             genCommaList(p, LEFT(rv));
Philosopher
 
Posts: 1593
Joined: Sun Aug 12, 2012 7:29 pm

Re: Nasal bugfixes - for 2.12?

Postby Hooray » Mon Jul 22, 2013 3:30 pm

For sanity checking, you can also use Andy's original regression tests available in the github repo (or the nasal-standalone repo at gitorious).
Please don't send support requests by PM, instead post your questions on the forum so that all users can contribute and benefit
Thanks & all the best,
Hooray
Help write next month's newsletter !
pui2canvas | MapStructure | Canvas Development | Programming resources
Hooray
 
Posts: 12707
Joined: Tue Mar 25, 2008 9:40 am
Pronouns: THOU

Re: Nasal bugfixes - for 2.12?

Postby Philosopher » Mon Jul 22, 2013 4:14 pm

Here's it in \n-escaped form (since phpBB does funny things when copy/pasting):
Code: Select all
diff --git a/src/codegen.c b/src/codegen.c\nindex 059c114..f309107 100644\n--- a/src/codegen.c\n+++ b/src/codegen.c\n@@ -147,7 +147,11 @@ static void genEqOp(int op, struct Parser* p, struct Token* t)\n\n static int defArg(struct Parser* p, struct Token* t)\n {\n-    if(t->type == TOK_LPAR) return defArg(p, RIGHT(t));\n+    if(t->type == TOK_LPAR) {\n+        if(LEFT(t) != RIGHT(t))\n+            naParseError(p, "cannot have default argument as function call", t->line);\n+        return defArg(p, RIGHT(t));\n+    }\n     if(t->type == TOK_MINUS && RIGHT(t) &&\n        RIGHT(t)->type == TOK_LITERAL && !RIGHT(t)->str)\n     {\n@@ -542,7 +546,7 @@ static void genAssign(struct Parser* p, struct Token* t)\n     if(parListLen(lv) || (lv->type == TOK_VAR && parListLen(RIGHT(lv)))) {\n         if(lv->type == TOK_VAR) { lv = RIGHT(lv); var = 1; }\n         len = parListLen(lv);\n-        if(rv->type == TOK_LPAR) {\n+        if(LEFT(rv) == RIGHT(rv) && rv->type == TOK_LPAR) {\n             if(len != parListLen(rv))\n                 naParseError(p, "bad assignment count", rv->line);\n             genCommaList(p, LEFT(rv));
Philosopher
 
Posts: 1593
Joined: Sun Aug 12, 2012 7:29 pm

Re: Nasal bugfixes - for 2.12?

Postby Hooray » Mon Jul 22, 2013 4:21 pm

use codepad (or some other pastebin).
Please don't send support requests by PM, instead post your questions on the forum so that all users can contribute and benefit
Thanks & all the best,
Hooray
Help write next month's newsletter !
pui2canvas | MapStructure | Canvas Development | Programming resources
Hooray
 
Posts: 12707
Joined: Tue Mar 25, 2008 9:40 am
Pronouns: THOU

Re: Nasal bugfixes - for 2.12?

Postby zakalawe » Tue Jul 23, 2013 9:51 am

I've pushed similar, but slightly different, fixes for both of these to 'next' now. Will port to the release branch once people have confirmed non-explosion of the world.

The change from your code, Philosopher, is to disambiguate multi-assignment vs function evaluation using the token's rule precedence instead of simply comparing the left and right tokens. This should work for functions with arguments too, and it's the idiom Andy was already using in another place to disambiguate. (At least, that's what I've managed to figure out by looking at the code).

Please pull & test on next, and let me know so I can merge to the release branch. In general default argument parsing seems a little strange to me, but I don't want to start making larger changes to the Nasal runtime now :)
zakalawe
 
Posts: 1259
Joined: Sat Jul 19, 2008 5:48 pm
Location: Edinburgh, Scotland
Callsign: G-ZKLW
Version: next
OS: Mac

Re: Nasal bugfixes - for 2.12?

Postby Philosopher » Tue Jul 23, 2013 2:32 pm

Hi James, thanks for committing :). In general it works fine, I'm just concerned that t->rule is never initialized and is going to be random – the chance of it being PREC_SUFFIX is very low but I do see some random numbers when I print the rule for each token. Initializing it to 0 in lex.c::newToken fixes that.

zakalawe wrote in Tue Jul 23, 2013 9:51 am:In general default argument parsing seems a little strange to me

Me too. However, it does allow statements such as a=((((-1)))) ;).
Philosopher
 
Posts: 1593
Joined: Sun Aug 12, 2012 7:29 pm

Re: Nasal bugfixes - for 2.12?

Postby Hooray » Tue Jul 23, 2013 2:44 pm

indeed, very good catch :D
Please don't send support requests by PM, instead post your questions on the forum so that all users can contribute and benefit
Thanks & all the best,
Hooray
Help write next month's newsletter !
pui2canvas | MapStructure | Canvas Development | Programming resources
Hooray
 
Posts: 12707
Joined: Tue Mar 25, 2008 9:40 am
Pronouns: THOU

Re: Nasal bugfixes - for 2.12?

Postby Philosopher » Tue Jul 23, 2013 2:57 pm

Here's a patch for #587 (Hooray pointed it out to me):
Code: Select all
diff --git a/simgear/nasal/codegen.c b/simgear/nasal/codegen.c
index fa2ddc2..a86cc07 100644
--- a/simgear/nasal/codegen.c
+++ b/simgear/nasal/codegen.c
@@ -485,6 +485,10 @@ static int tokMatch(struct Token* a, struct Token* b)
 static void genBreakContinue(struct Parser* p, struct Token* t)
 {
     int levels = 1, loop = -1, bp, cp, i;
+    // http://code.google.com/p/flightgear-bugs/issues/detail?id=587
+    // Make sure we are inside of a loop
+    if(p->cg->loopTop <= 0)
+        naParseError(p, "break/continue outside of a valid loop", t->line);
     if(RIGHT(t)) {
         if(RIGHT(t)->type != TOK_SYMBOL)
             naParseError(p, "bad break/continue label", t->line);

(This time a proper diff against simgear :P)
Philosopher
 
Posts: 1593
Joined: Sun Aug 12, 2012 7:29 pm

Re: Nasal bugfixes - for 2.12?

Postby Hooray » Tue Jul 23, 2013 3:12 pm

lol, now you have already solved three long-standing bugs in a single day - BTW: I rebuilt SG+FG from source, not seeing any Nasal related issues here, but there's an excessive amount of time spent for rebuilding the navcache for 2.99, even on Linux systems - using log-level bulk, it's obvious that it's mostly because of POI processing, i.e. it taking several minutes while FG looks unresponsive - basically, first-time FG users may have to spend up to 10 minutes for FG to start up, depending on system power (10 minutes was on an i7 quad core with 2.7 ghz and 8 gb RAM).
Please don't send support requests by PM, instead post your questions on the forum so that all users can contribute and benefit
Thanks & all the best,
Hooray
Help write next month's newsletter !
pui2canvas | MapStructure | Canvas Development | Programming resources
Hooray
 
Posts: 12707
Joined: Tue Mar 25, 2008 9:40 am
Pronouns: THOU

Re: Nasal bugfixes - for 2.12?

Postby zakalawe » Tue Jul 23, 2013 9:48 pm

Hmm, if t->rule isn't reliably initialised, then my solution is dangerous (admittedly low probability) - I'll add the init to 0, and the fix for #587 looks good too.

Hooray, can you file a defect about the POI parsing, assign to me, and run a couple of tests at log-level=info (BULK and DEBUG tend to skew the timing since there is some logging overhead). It's certainly POI that is sometimes slow (moreso on Windows), but no one has been able to figure out why it's usually ok, but sometimes bad. The file is much smaller than other files (apt.dat) which we parse much faster, although there are some additional index queries I think.
zakalawe
 
Posts: 1259
Joined: Sat Jul 19, 2008 5:48 pm
Location: Edinburgh, Scotland
Callsign: G-ZKLW
Version: next
OS: Mac

Re: Nasal bugfixes - for 2.12?

Postby Hooray » Tue Jul 23, 2013 9:55 pm

Regarding POI processing, I have already created full logs, but they seem not overly informative:


navaid:3:/home/hooray/sources/flightgear/src/Navaids/NavDataCache.cxx:1255:nav.dat load took:12603
navaid:3:/home/hooray/sources/flightgear/src/Navaids/NavDataCache.cxx:1263:poi.dat load took:678341
navaid:3:/home/hooray/sources/flightgear/src/Navaids/NavDataCache.cxx:1272:awy.dat load took:41155
navaid:3:/home/hooray/sources/flightgear/src/Navaids/NavDataCache.cxx:170:cache rebuild took:784669msec

full log file at: http://codepad.org/30LHOFys

~/.fgfsrc
Code: Select all
--fg-root=/home/hooray/sources/fgroot
--airport=KSFO
--aircraft=ufo
--disable-ai-traffic
--prop:/sim/ai/enabled=0


Maybe, there's some SQLite env variable that can be set to dump even more stuff at the SQLite layer ? Like I said, that's on a fairly powerful system and came as a surprise to me, because I didn't expect to see this on Linux at all. If other people can confirm this being the case for them, too I'd seriously consider disabling POI in 2.12 by default until this is solved, simply because it could be a major showstopper for people entirely new to FG, if they have to wait between 5-10 minutes for FG to start up, while looking totally unresponsive ...

As previously mentioned elsewhere, I think we need to look into using SQLite profiling and/or "EXPLAIN QUERY PLAN" to get to the bottom of this, because this has been affecting a number of users/platforms during the last release cycles. Is there a single place where I can patch SQL statements to have the "EXPLAIN QUERY PLAN" prefix and register callbacks for profiling ?
Please don't send support requests by PM, instead post your questions on the forum so that all users can contribute and benefit
Thanks & all the best,
Hooray
Help write next month's newsletter !
pui2canvas | MapStructure | Canvas Development | Programming resources
Hooray
 
Posts: 12707
Joined: Tue Mar 25, 2008 9:40 am
Pronouns: THOU

Next

Return to Development

Who is online

Users browsing this forum: No registered users and 8 guests