277122 - XUL preprocessor produces wrong results for #ifdef..#elifdef..#else...#endif (where the first two parts are true?)

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Asaf Romano (gone)]

see also bug 265711.

The following code lived(s) in
http://lxr.mozilla.org/aviarybranch/source/browser/base/content/browser.js#1058:

#ifdef XP_MACOSX
   if (!event.metaKey)
#elifdef XP_UNIX
  if (!event.altKey)
#else
   if (!event.ctrlKey)
#endif

The result on OS X (where both XP_MACOSX and XP_UNIX are defined) was:
if (!event.metaKey)
if (!event.ctrlKey)

instead of: if (!event.metaKey)

Note: nested #ifs work fine.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

This fixes if/elif/else/endif for me.  Testing consisted of a few mostly-trivial tests of this sort of thing and of running it against <http://software.hixie.ch/utilities/unix/preprocessor/test.txt>.  The former tests passed, and all but #12, #19, and #21 of the latter passed.  For each of those tests, however, the current broken else-relative-to-previous-condition behavior applied, or we were dealing with an out-of-order else block (i.e., if/else/else).  This patch makes #else equivalent to #elif 1, and combined with proper condition handling this means that blocks after an #else until #endif are never ever executed.

I'm not sure whether this is the best way to do it, but it works for me and is no more complex (at least in my mind) than the previous way this worked.

I have not built Mozilla with this; I intend to do so before committing, if this patch passes review.  (I also plan to run it against some tests Axel sent to me.)  Given Hixie's test, I'm fairly confident in its correctness.

Comment 2

[Hixie (not reading bugmail)]

lgtm, but I didn't look too closely.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 247200 [details] [diff] [review]
Patch

Something noted by Axel's tests: there's one 'argument expected' test which checks for |if $variable|; it should be |if defined($variable)| so that #ifdef 0 and #elifdef 0 work correctly.  I pass all his other tests.

Comment 4

[Jeff Walden [:Waldo]]

(In reply to comment #3)
> there's one 'argument expected' test

Well, actually two: one for ifdef, one for ifndef.  (Axel, you may want to add an ifndef 0 test to your preprocessor test files, seeing as there isn't one currently.)

I did a Firefox (clobber) build with the patched preprocessor.pl; nothing appears immediately incorrect with the build, but the only real way to test would be to do a diff of every preprocessed file to check.

(It would be sort of nice if existing tests weren't one monolithic test but rather a bunch of smaller tests for specific behaviors to make finding the root issue with a preprocessor bug easier, but as long as the preprocessor passes the entirety of the test it's less of a problem.  Implementing a preprocessor using the behavior description and the single monolithic test looks like it would be a small nightmare :-\ .)

Comment 5

[Axel Hecht [:Pike]]

Actually, I added 
FAIL 1
etc to my tests, which makes it a tad easier. I try to group them by functionality, too, so I can implement in chunks.

Re the ifndef 0 test, that should go into a quirks.in, as that it is more of an implementation detail than anything else. Or rather, ifdef 1 being true is.

Benjamin, should I file a bug on the tests that I have, or attach them here? Seems like they'd be of general use. I currently have them in build/tests, I'm open for other suggestions.

Comment 6

[Axel Hecht [:Pike]]

Attachment: bunch of testcases to be run with make checks in build/tests

Benjamin, Jeff, here goes another take on testcases for the preprocessor.

It's the suite of tests that I use to verify the perl written version against my python port, together with test.txt, test/test.txt from http://software.hixie.ch/utilities/unix/preprocessor/

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Previous patch, against CVS tree

I tried applying the previous patch, but it didn't quite seem to work because the wildcard was selecting full paths, and the paths in DIST_FILES need to be relative to srcdir.  A little hackery to the command (and adding build/tests to DIRS in build/) makes it work for me now, tho I highly doubt the way I did it is the correct way.  (I also had to change 'checks:' to 'check::'.)

Also, the license header for build/tests/Makefile.in is an old Netscape header and should be updated to be correct (what exactly "correct" is I don't know, so I didn't change it).

On a technical note, I'd prefer we did better testing of the JavaScript line stuff (exact output comparison would be preferable), but given how I understand CVS deals with line endings I'm not sure that's easy to do.

Comment 8

[Axel Hecht [:Pike]]

(In reply to comment #7)
> Created an attachment (id=247639) [edit]
> Previous patch, against CVS tree

So now we do have a build/tests directory in the cvs rep. I'm not sure if Benjamin likes that dir, thus I didn't cvs add that dir yet.

> I tried applying the previous patch, but it didn't quite seem to work because
> the wildcard was selecting full paths, and the paths in DIST_FILES need to be
> relative to srcdir.  A little hackery to the command (and adding build/tests to
> DIRS in build/) makes it work for me now, tho I highly doubt the way I did it
> is the correct way.  (I also had to change 'checks:' to 'check::'.)

I had no problems with that, I'll cross check.

> Also, the license header for build/tests/Makefile.in is an old Netscape header
> and should be updated to be correct (what exactly "correct" is I don't know, so
> I didn't change it).

Oops, I will.

> On a technical note, I'd prefer we did better testing of the JavaScript line
> stuff (exact output comparison would be preferable), but given how I understand
> CVS deals with line endings I'm not sure that's easy to do.

This is not an issue of line endings, but that the //@line directives contain the full path of the source file, which depends on your source tree. It would involve some pretty involved postprocessing to actually be able to check that fully. Or we'd do a javascript.out.in, and postprocess that with -Freplace and use @DIRECTORY@ all over. And then just cmp, that could work.

Comment 9

[Benjamin Smedberg]

Comment on attachment 247639 [details] [diff] [review]
Previous patch, against CVS tree

>Index: build/tests/Makefile.in

>+check::
>+	$(MAKE) libs DIST_FILES="$(subst $(srcdir)/,,$(wildcard $(srcdir)/[a-z]*.in))" FINAL_TARGET=.
>+	if grep FAIL *.in; then false; else echo "PASS"; fi

Please let me know what this code was meant to do: there has to be a better way: $(wildcard) is your friend

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 247639 [details] [diff] [review]
Previous patch, against CVS tree

> >+check::
> >+	$(MAKE) libs DIST_FILES="$(subst $(srcdir)/,,$(wildcard $(srcdir)/[a-z]*.in))" FINAL_TARGET=.
> >+	if grep FAIL *.in; then false; else echo "PASS"; fi
> 
> Please let me know what this code was meant to do: there has to be a better
> way: $(wildcard) is your friend

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/config/rules.mk&rev=3.533&mark=1715-1723#1710

make libs runs all the files in the space-separated list stored in DIST_FILES through the preprocessor, and it spits out the preprocessed versions to FINAL_TARGET.  The problem is obvious when you look at line 1721: the file being preprocessed is assumed to be a path relative to $(srcdir), and so the rule prepends $(srcdir) to the location.  Since the file list already has $(srcdir) in it, all the paths when they're given to the preprocessor script on the command line are of the form $(srcdir)/$(srcdir)/file-to-preprocess.in.  This is one too many $(srcdir)/s, so the preprocessor dies.  My hack fix was to generate the list just as before but to remove the leading $(srcdir)/ from the paths in DIST_FILES (substitute $(srcdir)/ with the empty string).  The files are then paths relative to $(srcdir), and everything's happy.

One fix would be to list the files individually in the Makefile, which is the easiest solution; on the other hand, I think automagical behavior is something we want here.

Comment 11

[Benjamin Smedberg]

Comment on attachment 247200 [details] [diff] [review]
Patch

Quick question: if I have the following construct

#if 0
FOO
#else
BAR
#else
BAZ
#endif

Does that throw an error, or just print BAR?

Comment 12

[Jeff Walden [:Waldo]]

(In reply to comment #11)
> Does that throw an error, or just print BAR?

It just prints BAR, because #else is equivalent to #elif 1.  Basically, any #else, #elif, etc. after an #else is just so much dead code -- anything after an #else up to the corresponding closing #endif will never be executed or included -- and no error will be thrown.  The preprocessor as it exists now makes almost no effort to ensure you don't shoot yourself in the foot while using it, and this patch continues that dubious tradition.  (Error handling would be nice, but I think it's a separate bug if someone wants to file/fix it.)

Comment 13

[Axel Hecht [:Pike]]

(In reply to comment #9)
> Please let me know what this code was meant to do: there has to be a better
> way: $(wildcard) is your friend

I was looking for a rule that would run the preprocessor in a standard way and output into builddir. For an in-src-dir build, this is probably borked, now that you mention it. Could we factor out the preprocessor calling stuff in rules.mk to a single define, so that I could reuse that in the makefile?

What I really want to do is, process all foo.in (besides Makefile.in) into foo, and see if the string FAIL pops up in the output. I'd rather have the real files there, as some tests don't really fail if they fail, in particular the javascript line ending stuff.
The if grep ... is basically just to negate the grep return value.

Right now, I'm more interested if the test location and the test setup is right than what the Makefile looks like to run them, too.

Comment 14

[Jeff Walden [:Waldo]]

Patch checked in on trunk, albeit after a few hiccups: it turns out that Perl's |use constant { FOO => 1, BAR => 2 }| syntax is >=5.8.0, which broke balsa-trunk which has 5.6.0.  I switched to the older syntax |use constant FOO => 1;|, and tinderboxen seem to be handling the patch okay now.

Leaving open to handle getting a test framework in place (and then getting some tests for the new, correct behavior committed as well)...

Comment 15

[Jeff Walden [:Waldo]]

preprocessor.pl is mostly dead and replaced with the Python replacement (which does have tests), so this doesn't really need to stay open any more.