Sylvestre Ledru [:Sylvestre] Assignee
	Description • 6 years ago

In file included from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/parser/html/Unified_cpp_parser_html1.cpp:92:
 /root/firefox-gcc-last/parser/html/nsHtml5TreeBuilder.cpp: In member function 'void nsHtml5TreeBuilder::characters(const char16_t*, int32_t, int32_t)':
 /root/firefox-gcc-last/parser/html/nsHtml5TreeBuilder.cpp:349:13: error: this statement may fall through [-Werror=implicit-fallthrough=]
              switch (mode) {
              ^~~~~~
 /root/firefox-gcc-last/parser/html/nsHtml5TreeBuilder.cpp:404:11: note: here
            default: {
            ^~~~~~~

I guess this is the same as bug 1426997.

	Chris Peterson [:cpeterson]
	Comment 2 • 6 years ago

nsHtml5TreeBuilder.cpp is a generated filed, so the fix needs to be made in the HTML5 parser written in Java. See bug 1426997 comment 11 and 12.

Also, does this fallthrough case need to assert in debug builds? The HTML5 parser's fallthrough cases that I annotated were all intentional, so I used MOZ_FALLTHROUGH instead of MOZ_FALLTHROUGH_ASSERT. I wonder why this particular case was caught by gcc 8 but not by clang's -Wimplicit-fallthrough warning.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 3 • 6 years ago

It appears that this line indeed in unreachable, so this would be r+ in that sense. However, recent experience indicates that if I say "r+, but please land the corresponding htmlparser repo fix", the htmlparser repo fix doesn't get landed. OTOH, I don't want to make people jump through the hoops of setting up HTML parser regeneration just to change one line. (Aside: Now that we can require Node to build Firefox, should we also require OpenJDK so as not to require special setup for the regeneration...)

Are you OK with doing the htmlparser repo landing or would you like me to do it?

To assert unreachability in Java, we can just say:

assert false;

which should translate into a MOZ_ASSERT. However, if gcc isn't happy enough with the specific kind of assertion that gets generated and still complains about fall-through, I guess we could say:

assert false;
// CPPONLY: MOZ_FALLTHROUGH_ASSERT("unreachable");

	Sylvestre Ledru [:Sylvestre] Assignee
	Comment 4 • 6 years ago

I will do in the java parser too. No worries, thanks!

	Sylvestre Ledru [:Sylvestre] Assignee
	Comment 5 • 6 years ago

The java change.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 7 • 6 years ago
mozreview-review

Comment on attachment 8964597 [details]
Bug 1447480 - Add a MOZ_ASSERT to make it clear that it can never fall through

https://reviewboard.mozilla.org/r/233320/#review239076

::: parser/html/nsHtml5TreeBuilder.cpp:403
(Diff revision 1)
>                  reconstructTheActiveFormattingElements();
>                  continue;
>                }
>              }
> +            MOZ_ASSERT(false);
> +            MOZ_FALLTHROUGH_ASSERT(nsGkAtoms::unreachable);

Does this actually compile? The simplest way forward would be to use the macro without an argument. (And to make the macro support the no-argument case if not already supported.)

	Chris Peterson [:cpeterson]
	Comment 8 • 6 years ago

> ::: parser/html/nsHtml5TreeBuilder.cpp:403
> > +            MOZ_ASSERT(false);
> > +            MOZ_FALLTHROUGH_ASSERT(nsGkAtoms::unreachable);

MOZ_ASSERT(false) is redundant here. MOZ_FALLTHROUGH_ASSERT will (do the equivalent of) MOZ_ASSERT(false) in debug builds.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 11 • 6 years ago
mozreview-review

Comment on attachment 8964597 [details]
Bug 1447480 - Add a MOZ_ASSERT to make it clear that it can never fall through

https://reviewboard.mozilla.org/r/233320/#review239180

	Pulsebot
	Comment 12 • 6 years ago

Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fa109105e4ea
Add a MOZ_ASSERT to make it clear that it can never fall through r=hsivonen

	Sylvestre Ledru [:Sylvestre] Assignee
	Comment 13 • 6 years ago

https://hg.mozilla.org/projects/htmlparser/rev/2620240e520b0ea8a1b15d9a163c7d80dcf6c673
Bug 1447480 - Add a MOZ_ASSERT to make it clear that it can never fall through r?hsivonen

	Eliza Balazs [:ebalazs_]
	Comment 14 • 6 years ago

Backed out changeset fa109105e4ea (bug 1447480) for B failures in /build/build/src/parser/html/nsHtml5TreeBuilder.cpp(402)

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=fa109105e4eadb31fe8a1bedf6f644fdc290e897&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success&selectedJob=171829309

Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=45374e43b96074c9d8ec4a86a5c7ae008d26e208&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success&filter-resultStatus=retry&filter-resultStatus=running&filter-resultStatus=pending

Failure log: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=45374e43b96074c9d8ec4a86a5c7ae008d26e208&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success&filter-resultStatus=retry&filter-resultStatus=running&filter-resultStatus=pending

	Sylvestre Ledru [:Sylvestre] Assignee
	Comment 15 • 6 years ago

> Does this actually compile? The simplest way forward would be to use the
> macro without an argument. 
"not enough actual parameters for macro 'MOZ_FALLTHROUGH_ASSERT'"

Looks like it isn't :)

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 17 • 6 years ago
mozreview-review

Comment on attachment 8965253 [details]
Bug 1447480 - Add support of MOZ_FALLTHROUGH_ASSERT without any argument

https://reviewboard.mozilla.org/r/233964/#review239594

Seems reasonable to me, but I'm not officially a peer for this code.

	Pulsebot
	Comment 18 • 6 years ago

Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a454ed4489f2
Add a MOZ_ASSERT to make it clear that it can never fall through r=hsivonen
https://hg.mozilla.org/integration/autoland/rev/5dfbd42ce515
Add support of MOZ_FALLTHROUGH_ASSERT without any argument r=hsivonen

	Pulsebot
	Comment 19 • 6 years ago

Backout by toros@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e92d8ad7c1a0
Backed out 2 changesets for build bustages at dist/include/mozilla/Assertions.h:60 a=backout on a CLOSED TREE

	Tiberius Oros[:tiberius_oros]
	Comment 20 • 6 years ago

Backed out 2 changesets (bug 1447480) for build bustages at dist/include/mozilla/Assertions.h:60

https://hg.mozilla.org/integration/autoland/rev/e92d8ad7c1a0

https://treeherder.mozilla.org/logviewer.html#?job_id=172047636&repo=autoland&lineNumber=3149

	Sylvestre Ledru [:Sylvestre] Assignee
	Comment 21 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=138153b71b519f21dfb65e02225ba07c19b0b928

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 23 • 6 years ago
mozreview-review

Comment on attachment 8965253 [details]
Bug 1447480 - Add support of MOZ_FALLTHROUGH_ASSERT without any argument

https://reviewboard.mozilla.org/r/233964/#review240530

I suppose the result is good enough even if not great.

	Pulsebot
	Comment 24 • 6 years ago

Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/639322e83c65
Add a MOZ_ASSERT to make it clear that it can never fall through r=hsivonen
https://hg.mozilla.org/integration/autoland/rev/1fdb4dcb4225
Add support of MOZ_FALLTHROUGH_ASSERT without any argument r=hsivonen

	Noemi Erli[:noemi_erli]
	Comment 25 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/639322e83c65
https://hg.mozilla.org/mozilla-central/rev/1fdb4dcb4225