Closed Bug 1019955 Opened 6 years ago Closed 6 years ago

"Start server" getting into a self-hosted file?

Categories

(Core :: JavaScript Engine, defect, blocker)

defect
Not set
blocker

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: sfink, Assigned: till)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

https://tbpl.mozilla.org/php/getParsedLog.php?id=40983644&tree=Mozilla-Inbound&full=1 contains the error message

Executing /builds/slave/m-in-lx-0000000000000000000000/build/obj-firefox/dist/bin/xpcshell -g /builds/slave/m-in-lx-0000000000000000000000/build/obj-firefox/dist/bin/ -a /builds/slave/m-in-lx-0000000000000000000000/build/obj-firefox/dist/bin/ -f /builds/slave/m-in-lx-0000000000000000000000/build/toolkit/mozapps/installer/precompile_cache.js -e precompile_startupcache("resource://gre/");
self-hosted:1:6 SyntaxError: missing ; before statement:
self-hosted:1:6 Start server
self-hosted:1:6 ......^

It seems something is printing "Start server" on its output and that is making it into something that should be a source file.

This has hit a few times in the last week.
I've seen this before about 10 days ago.

Changeset 91eb27108c0b was backed out for it, but the same thing happened again on the backout: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=11090a948663

Relanding with only a wholly-unrelated change before worked just fine: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=315b244f4e90

I don't know the details, but according to Tomcat this was infrastructure-related. Carsten, do you know more about what went on there?
Flags: needinfo?(cbook)
Highly doubt this is infra. It's specifically tied to patches that touch self-hosting.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #2)
> Highly doubt this is infra. It's specifically tied to patches that touch
> self-hosting.

I do see that there is a correlation there, but I don't understand it. Why would it be tied to a self-hosting-touching patch, then not go away upon backout, but not exist on the next landing of said patch? How would the first landing of the patch be any different from subsequent builds with the change included?
We go to a lot of lengths to recycle builders as much as possible in order to minimize build times (by reusing an already-current objdir). So in fact, it makes perfect sense. The underlying issue is there regardless of the patch that manages to tickle it. So both the original landing and the backout can tickle the same problem. The only sure-fire way to make it go away is to clobber and clear that bad-state objdir and force the self-hosted stuff to rebuild.
yeah in the case till mentioned it was i think the mix case of clobber and clear the bad state possible. When i tried to check the slave that failed on backout and original checkin the slave went away (was not reachable and so a new slave finally took over on retrigger - so maybe this was the case with kind of clobber here :)
Flags: needinfo?(cbook)
This needs attention ASAP as it's a cause of frequent, unpredictable bustage.
Severity: normal → blocker
Flags: needinfo?(till)
Is it truly unpredictable?  Without fully understanding the issue, but observing the self-hosting connection, I would expect this to trigger for changes to anything listed in selfhosted_out_h_deps in js/src/Makefile.in.  That list is not small.  But it's not all that large, either -- particularly if you consider the js/src/builtin/*.js files as basically one conceptual category.  For now I would exercise preemptive CLOBBER for anything that touches those files, while still keeping priority bumped here.
How are you planning to effectively communicate that to *all* JS devs?
Constant vigilance, reviewer diligence.  It's not a large set of people who do reviews, for the most part.  We can post to js-internals and pick up most people that way, I expect.
Sent mail to js-internals newsgroup/list about this.  And I'll poke people on IRC when I head to the office shortly, too.
This is believed responsible for the currently mass B2G desktop failures on b2g-inbound, which is holding the tree closed until clobbered build retriggers (and the associated subsequent tests) come back green - which will be several hours.
Blocks: clobber
https://hg.mozilla.org/mozilla-central/rev/ca77e129db83
Assignee: nobody → ryanvm
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Assignee: ryanvm → nobody
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla32 → ---
I'm pretty sure this is because of sccache: https://github.com/glandium/sccache/blob/master/sccache.py#L51

What embedjs.py (the script that processes all the self-hosted files) does is take a path to cpp, the C preprocessor, and saves the stdout that cpp gives back.

The path to cpp on TBPL looks like: 'python2.7 /builds/slave/m-in-l64-000000000000000000000/build/sccache/sccache.py /builds/slave/m-in-l64-000000000000000000000/build/gcc/bin/gcc -E'

Every once in a while, sccache.py prints out 'Start server' to stdout, which gets slurped up by embedjs.py

The easiest fix would make sccache.py print 'Start server' on stderr or something. I don't remember why embedjs.py pipes cpp's stdout instead of making stdout write to a temporary file. I think perhaps because I couldn't find a flag for it for MSVC or something?
Flags: needinfo?(till) → needinfo?(mh+mozilla)
s/making stdout write to a temporary file/making cpp write to a temporary file
I think this should do the trick. Try-servering here:
https://tbpl.mozilla.org/?tree=Try&rev=674c4d7149fc

Should also fix bug 980793.
Attachment #8435418 - Flags: review?(mh+mozilla)
Assignee: nobody → till
Status: REOPENED → ASSIGNED
Comment on attachment 8435418 [details] [diff] [review]
Write preprocessing results for self-hosted code into file instead of stdout

Review of attachment 8435418 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/embedjs.py
@@ +86,2 @@
>      output.write('\n'.join([msgs] + ['#include "%(s)s"' % { 's': source } for source in sources]))
> +  cmdline = cxx + preprocessorOption + [processedSources] +\

I don't expect this to work on Windows: the command line argument is -Feoutputfile, not -Fe outputfile.

@@ +132,5 @@
>  #include "%(msgs)s"
>  """ % { 'msgs': msgs })
> +  p = subprocess.check_output(cxx + preprocessorOption + [processedMsgs] + [msgsTemplate])
> +  with open(processedMsgs, 'r') as input:
> +    processed = input.read()

You should have a function to do preprocessing instead of having two manual invocations of subprocess.check_output.
Attachment #8435418 - Flags: review?(mh+mozilla) → review-
Right you are. This is somewhat refactored and should fare better: https://tbpl.mozilla.org/?tree=Try&rev=4b38349c9e08
Attachment #8435719 - Flags: review?(mh+mozilla)
Attachment #8435418 - Attachment is obsolete: true
Comment on attachment 8435719 [details] [diff] [review]
Write preprocessing results for self-hosted code into file instead of stdout. v2

Ok, the Windows build still doesn't succeed. I don't understand what's going on there, yet, but am looking into it.
Attachment #8435719 - Flags: review?(mh+mozilla)
Flags: needinfo?(mh+mozilla)
Ok, so I really don't understand what's going on here. I thought that maybe the relative paths or something about using check_output instead of Popen is responsible for the broken builds on Windows, but my latest try push doesn't do either of those:
https://tbpl.mozilla.org/?tree=Try&rev=011202b670d5

It works on my Windows machine, and everywhere else, but the build machine complains "WindowsError: [Error 2] The system cannot find the file specified". I don't even know which file is meant or how to debug this.

Glandium, can you take a look at the patch[1] and the log to see if you know what's going on?


[1]: https://hg.mozilla.org/try/rev/4487b82856d0
[2]: https://tbpl.mozilla.org/php/getParsedLog.php?id=41438902&tree=Try&full=1#error4
Flags: needinfo?(mh+mozilla)
Comment on attachment 8435719 [details] [diff] [review]
Write preprocessing results for self-hosted code into file instead of stdout. v2

Review of attachment 8435719 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/embedjs.py
@@ +119,2 @@
>    # Clang seems to complain and not output anything if the extension of the
> +  # input is not something it recognizes, so just fake a .cpp here.

It'd be a little cleaner to pass in |-x c++|
(In reply to Steve Fink [:sfink] from comment #21)
> It'd be a little cleaner to pass in |-x c++|

True, but that doesn't work with VSCC :( There's probably an equivalent, but we'd have to pass the flag to use in and set up the correct build script var in the first place, etc.
http://bugs.python.org/issue8557

You need to resolve the path of the program before executing it (use which.which, it's in the virtualenv)
Flags: needinfo?(mh+mozilla)
Thanks again for the help over IRC. Try-servering with which.which now: https://tbpl.mozilla.org/?tree=Try&rev=90ddce49150f
Attachment #8438235 - Flags: review?(mh+mozilla)
Attachment #8435719 - Attachment is obsolete: true
Comment on attachment 8438235 [details] [diff] [review]
Write preprocessing results for self-hosted code into file instead of stdout. v3

Review of attachment 8438235 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/embedjs.py
@@ +90,5 @@
> +  combinedSources = '\n'.join([msgs] + ['#include "%(s)s"' % { 's': source } for source in sources])
> +  args = ['-D%(k)s=%(v)s' % { 'k': k, 'v': env[k] } for k in env]
> +  preprocessed = preprocess(cxx, preprocessorOption, combinedSources, args)
> +  processed = '\n'.join([line for line in preprocessed.splitlines() if not\
> +                         (len(line) == 0 or line.isspace() or line.startswith('#'))])

This condition can be written as if line.strip() and not line.startswith('#') which might be more readable. ymmv.

@@ +121,4 @@
>    # Clang seems to complain and not output anything if the extension of the
> +  # input is not something it recognizes, so just fake a .cpp here.
> +  if (not os.path.exists(cxx[0])):
> +    cxx[0] = which.which(cxx[0])

You may want to move this above the comment so that the comment stays near the code it applies to.

@@ +124,5 @@
> +    cxx[0] = which.which(cxx[0])
> +  tmpIn = 'self-hosting-cpp-input.cpp';
> +  tmpOut = 'self-hosting-preprocessed.pp';
> +  outputArg = shlex.split(preprocessorOption)
> +  outputArg[-1] += tmpOut

outputArg = shlex.split(preprocessorOption + tmpOut) would be more accurate.

@@ +126,5 @@
> +  tmpOut = 'self-hosting-preprocessed.pp';
> +  outputArg = shlex.split(preprocessorOption)
> +  outputArg[-1] += tmpOut
> +
> +  with open(tmpIn, 'wb') as output:

input

@@ +131,5 @@
> +    output.write(source)
> +  print(' '.join(cxx + outputArg + args + [tmpIn]))
> +  result = subprocess.Popen(cxx + outputArg + args + [tmpIn]).wait()
> +  if (result != 0):
> +    exit(result);

sys.exit

@@ +132,5 @@
> +  print(' '.join(cxx + outputArg + args + [tmpIn]))
> +  result = subprocess.Popen(cxx + outputArg + args + [tmpIn]).wait()
> +  if (result != 0):
> +    exit(result);
> +  with open(tmpOut, 'r') as input:

output
Attachment #8438235 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/2e3a1b454899
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1028678
Depends on: 1029811
You need to log in before you can comment on or make changes to this bug.