1434765 - The preprocessor should reject #idef clauses with expressions

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

Bug 1434761 is a case where this came up and dumb copying and pasting could have made this bug proliferate. In bug 1432310 comment 13 glandium suggests we can do better.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1434765 - Properly reject invalid variables in #if{,n}def.

The invalid variable test for #if{,n}def was only checking that the
first character in the variable was alphanumeric or underscore, not
the other characters.

More generally, preprocessor instructions were also cut out such that
whitespaces before and after arguments were part of the arguments.

Review commit: https://reviewboard.mozilla.org/r/217050/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217050/

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8947328 [details]
Bug 1434765 - Properly reject invalid variables in #if{,n}def.

https://reviewboard.mozilla.org/r/217050/#review222890

::: python/mozbuild/mozbuild/test/test_preprocessor.py:651
(Diff revision 1)
> +        with MockedOpen({'dummy': '#ifdef FOO BAR\nPASS\n#endif'}):
> +            with self.assertRaises(Preprocessor.Error) as e:
> +                self.pp.do_include('dummy')
> +            self.assertEqual(e.exception.key, 'INVALID_VAR')
> +
> +        with MockedOpen({'dummy': '#ifndef FOO BAR\nPASS\n#endif'}):

Bravo for writing tests, too.

Do you want to include a test for `FOO == BAR`, which motivated this bug?

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8947328 [details]
Bug 1434765 - Properly reject invalid variables in #if{,n}def.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217050/diff/1-2/

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1434765 - Fix #ifdef in layout/tools/reftest/manifest.jsm.

Review commit: https://reviewboard.mozilla.org/r/217074/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217074/

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8947328 [details]
Bug 1434765 - Properly reject invalid variables in #if{,n}def.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217050/diff/2-3/

Comment 6

[Mike Hommey [:glandium]]

Bwarf, the printing tests that were not happening because of the malformed preprocessor clause are actually busted. They might have been busted since day one, but nobody knew.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8947328 [details]
Bug 1434765 - Properly reject invalid variables in #if{,n}def.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217050/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4582a286d6a
Properly reject invalid variables in #if{,n}def. r=froydnj

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/d4582a286d6a

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for breaking build of Catalan language pack due to altered output:

https://hg.mozilla.org/mozilla-central/rev/bda9adefe73902685d6689a205e7114ae9df7f83

Push with broken Catalan Firefox for Android nightlies (desktop hasn't run yet): https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=a23212941fcd1d8ecc2862cc139dfcbb7f029aa2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=159919431&repo=mozilla-central

[task 2018-02-01T19:13:04.490Z] 19:13:04     INFO -  make[1]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/mobile/android/locales'
[task 2018-02-01T19:13:04.491Z] 19:13:04     INFO -  /builds/worker/workspace/build/src/obj-firefox/_virtualenv/bin/python /builds/worker/workspace/build/src/config/nsinstall.py -D ../../../dist/android-arm/xpi/
[task 2018-02-01T19:13:04.526Z] 19:13:04     INFO -  /builds/worker/workspace/build/src/obj-firefox/_virtualenv/bin/python -m mozbuild.action.langpack_manifest --locales ca --min-app-ver 60.0a1 --max-app-ver 60.0a1 --app-name "Firefox Nightly" --l10n-basedir "/builds/worker/workspace/build/l10n-central" --defines /builds/worker/workspace/build/l10n-central/ca/toolkit/defines.inc /builds/worker/workspace/build/l10n-central/ca/mobile/android/defines.inc  --input ../../../dist/xpi-stage/locale-ca
[task 2018-02-01T19:13:04.962Z] 19:13:04    ERROR -  Traceback (most recent call last):
[task 2018-02-01T19:13:04.963Z] 19:13:04     INFO -    File "/usr/lib/python2.7/runpy.py", line 174, in _run_module_as_main
[task 2018-02-01T19:13:04.969Z] 19:13:04     INFO -      "__main__", fname, loader, pkg_name)
[task 2018-02-01T19:13:04.970Z] 19:13:04     INFO -    File "/usr/lib/python2.7/runpy.py", line 72, in _run_code
[task 2018-02-01T19:13:04.970Z] 19:13:04     INFO -      exec code in run_globals
[task 2018-02-01T19:13:04.970Z] 19:13:04     INFO -    File "/builds/worker/workspace/build/src/python/mozbuild/mozbuild/action/langpack_manifest.py", line 450, in <module>
[task 2018-02-01T19:13:04.970Z] 19:13:04     INFO -      main(sys.argv[1:])
[task 2018-02-01T19:13:04.970Z] 19:13:04     INFO -    File "/builds/worker/workspace/build/src/python/mozbuild/mozbuild/action/langpack_manifest.py", line 444, in main
[task 2018-02-01T19:13:04.971Z] 19:13:04     INFO -      chrome_entries
[task 2018-02-01T19:13:04.971Z] 19:13:04     INFO -    File "/builds/worker/workspace/build/src/python/mozbuild/mozbuild/action/langpack_manifest.py", line 409, in create_webmanifest
[task 2018-02-01T19:13:04.971Z] 19:13:04     INFO -      return json.dumps(manifest, indent=2, ensure_ascii=False, encoding='utf8')
[task 2018-02-01T19:13:04.971Z] 19:13:04     INFO -    File "/usr/lib/python2.7/json/__init__.py", line 251, in dumps
[task 2018-02-01T19:13:04.971Z] 19:13:04     INFO -      sort_keys=sort_keys, **kw).encode(obj)
[task 2018-02-01T19:13:04.972Z] 19:13:04     INFO -    File "/usr/lib/python2.7/json/encoder.py", line 209, in encode
[task 2018-02-01T19:13:04.972Z] 19:13:04     INFO -      chunks = list(chunks)
[task 2018-02-01T19:13:04.972Z] 19:13:04     INFO -    File "/usr/lib/python2.7/json/encoder.py", line 434, in _iterencode
[task 2018-02-01T19:13:04.972Z] 19:13:04     INFO -      for chunk in _iterencode_dict(o, _current_indent_level):
[task 2018-02-01T19:13:04.972Z] 19:13:04     INFO -    File "/usr/lib/python2.7/json/encoder.py", line 390, in _iterencode_dict
[task 2018-02-01T19:13:04.973Z] 19:13:04     INFO -      yield _encoder(value)
[task 2018-02-01T19:13:04.973Z] 19:13:04     INFO -    File "/usr/lib/python2.7/json/encoder.py", line 233, in _encoder
[task 2018-02-01T19:13:04.973Z] 19:13:04     INFO -      o = o.decode(_encoding)
[task 2018-02-01T19:13:04.973Z] 19:13:04     INFO -    File "/builds/worker/workspace/build/src/obj-firefox/_virtualenv/lib/python2.7/encodings/utf_8.py", line 16, in decode
[task 2018-02-01T19:13:04.973Z] 19:13:04     INFO -      return codecs.utf_8_decode(input, errors, True)
[task 2018-02-01T19:13:04.973Z] 19:13:04     INFO -  UnicodeDecodeError: 'utf8' codec can't decode byte 0xc3 in position 5: invalid continuation byte

gandalf found the difference:

without the patch pp.context is:
{'MOZ_LANGPACK_CONTRIBUTORS': '<em:contributor>Softcatal\xc3\xa0</em:contributor>', 'MOZ_LANGPACK_CREATOR': 'Mozilla.org / Softcatal\xc3\xa0', 'MOZ_LANG_TITLE': 'Catal\xc3\xa0', 'FILE': '', 'DIRECTORY': '/home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/browser/locales', 'LINE': 0}

with: 
{'MOZ_LANGPACK_CONTRIBUTORS': '<em:contributor>Softcatal\xc3\xa0</em:contributor>', 'MOZ_LANGPACK_CREATOR': 'Mozilla.org / Softcatal\xc3', 'MOZ_LANG_TITLE': 'Catal\xc3', 'FILE': '', 'DIRECTORY': '/home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/browser/locales', 'LINE': 0}

Comment 11

[Mike Hommey [:glandium]]

So here's why this is happening:

That defines.inc contains:

#define MOZ_LANGPACK_CREATOR mozilla.org / Softcatalà

That à, is encoded as 0xc3 0xa0. Guess what 0xa0 is in iso-8859-1?  . So the \s* that strips whitespaces removes it...

The core problem is that lines are being treated as strs, not unicodes. There may be legitimate cases for supporting preprocessing raw bytes, but it seems we should at least try to use unicode when the input is valid utf-8.

Comment 12

[Mike Hommey [:glandium]]

However, this happens to be a rabbit hole. Many things in the preprocessor are coercing values to str. Making it unicode aware seems like much more work than I'm ready to put at the moment. I think a reasonable way out here is to get rid of re.U.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8947328 [details]
Bug 1434765 - Properly reject invalid variables in #if{,n}def.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217050/diff/4-5/

Comment 14

[Mike Hommey [:glandium]]

Filed bug 1435087

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8947328 [details]
Bug 1434765 - Properly reject invalid variables in #if{,n}def.

https://reviewboard.mozilla.org/r/217050/#review224382

Let's try again!  Yay, Catalan!

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8947328 [details]
Bug 1434765 - Properly reject invalid variables in #if{,n}def.

https://reviewboard.mozilla.org/r/217050/#review224384

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd087947d886
Properly reject invalid variables in #if{,n}def. r=froydnj,nalexander

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/fd087947d886