Bug 162705 – Offline asm should not output masm assembly when using a x86_64 asm backend

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Christopher Reid]

The offline asm ruby scripts are incorrectly generating masm assembly when cross compiling to offline_arm_x86_64 from windows.

[Christopher Reid]

Created attachment 290137 [details]
Patch

[Christopher Reid]

Created attachment 290139 [details]
Patch2

Replacing the static method with function variables

[WebKit Commit Bot]

Attachment 290139 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/PlatformPlayStation.cmake:5:  Alphabetical sorting problem. "generic/MainThreadGeneric.cpp" should be before "text/jsconly/TextBreakIteratorInternalICUJSCOnly.cpp".  [list/order] [5]
ERROR: CMakeLists.txt:31:  Alphabetical sorting problem. "PlayStation4" should be before "WinCairo".  [list/order] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Christopher Reid]

Created attachment 290141 [details]
Patch3

Sorry, attached the wrong patch

[Christopher Reid]

Created attachment 290146 [details]
Patch

Adding the changelog

[Alex Christensen]

What is the context of this change.  We have 64-bit windows bots, and they are compiling and running fine.  Are you cross compiling or using a different assembler or something?

[Christopher Reid]

(In reply to comment #6)

We're currently cross compiling on Windows with Clang. Because of our different platform, the ruby script is using an active backend of x86_64 rather than x86_64_win but the isCompilingOnWindows is overriding this. This change is to make it so the script uses the active backend rather than just assuming masm when compiling on windows.

[Alex Christensen]

Looks good to me.  Mark, could you take a look?

[Mark Lam]

(In reply to comment #7)
> (In reply to comment #6)
> 
> We're currently cross compiling on Windows with Clang. Because of our
> different platform, the ruby script is using an active backend of x86_64
> rather than x86_64_win but the isCompilingOnWindows is overriding this. This
> change is to make it so the script uses the active backend rather than just
> assuming masm when compiling on windows.

I would like to see an explanation like this put in the ChangeLog to record the motivation for this change.  Something like:

"When cross compiling on Windows with Clang, ..."

That said, I have further questions and comments in the patch.  I'll comment there shortly.

[Christopher Reid]

Created attachment 290250 [details]
Patch4

Adding a description of the change as suggested.

[Mark Lam]

Comment on attachment 290146 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290146&action=review

> Source/JavaScriptCore/offlineasm/x86.rb:142
>  def isIntelSyntax
> -    isCompilingOnWindows
> +    isWin
>  end

It looks like this change is the only one that is of consequence.  All the other changes below only change the code to use a local var (initialized by a function) instead of a global var.  Are they really necessary?  Or did I misread something?

I see that the isGCC and isMSVC functions are also based on isCompilingOnWindows.  isGCC is not used anywhere.  isMSVC is used for (1) determining the offlineasm output file format and (2) whether to enable DWARF2 debug annotations.  If you're cross compiling with Clang for a non-Windows target, wouldn't you want these configured like other clang builds too?

This is what I would do:
1. remove isGCC because it is not used.
2. change isMSVC to be based on isWin.
3. remove isCompilingOnWindows since it will no longer be in use.

[Mark Lam]

Comment on attachment 290146 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290146&action=review

>> Source/JavaScriptCore/offlineasm/x86.rb:142
>>  end
> 
> It looks like this change is the only one that is of consequence.  All the other changes below only change the code to use a local var (initialized by a function) instead of a global var.  Are they really necessary?  Or did I misread something?
> 
> I see that the isGCC and isMSVC functions are also based on isCompilingOnWindows.  isGCC is not used anywhere.  isMSVC is used for (1) determining the offlineasm output file format and (2) whether to enable DWARF2 debug annotations.  If you're cross compiling with Clang for a non-Windows target, wouldn't you want these configured like other clang builds too?
> 
> This is what I would do:
> 1. remove isGCC because it is not used.
> 2. change isMSVC to be based on isWin.
> 3. remove isCompilingOnWindows since it will no longer be in use.

Also remove all the other unnecessary changes if they are indeed unnecessary.  Thanks.

[Christopher Reid]

(In reply to comment #12)

Thanks for your feedback Mark, 

> It looks like this change is the only one that is of consequence.  All the
> other changes below only change the code to use a local var (initialized by
> a function) instead of a global var.  Are they really necessary?  Or did I
> misread something?

What was happening there was that the activebackend was not being set until later on in the code when some node operations are done in backends.rb. In ruby, class and instance level variables are resolved when the file is included which was causing the generate offsets script to fail too.

> 2. change isMSVC to be based on isWin.
The same problem as above is seen here, isWin is called from asm.rb before the backend is set. For wincairo and other platforms that use cmake, the correct file name is set in cmake which causes emitWinAsm to be set correctly. DWARF2 annotations will still be incorrectly set for cross compiling to a x86_(_64)_win backend. It would be a nice change to have but it is much more involved. 

I agree with removing isGCC, I can make a patch for that. Let me know if you think it's still worthwhile to remove isMSVC.

[Mark Lam]

(In reply to comment #13)
> (In reply to comment #12)
> 
> Thanks for your feedback Mark, 
> 
> > It looks like this change is the only one that is of consequence.  All the
> > other changes below only change the code to use a local var (initialized by
> > a function) instead of a global var.  Are they really necessary?  Or did I
> > misread something?
> 
> What was happening there was that the activebackend was not being set until
> later on in the code when some node operations are done in backends.rb. In
> ruby, class and instance level variables are resolved when the file is
> included which was causing the generate offsets script to fail too.

OK.  Sounds good.

> > 2. change isMSVC to be based on isWin.
> The same problem as above is seen here, isWin is called from asm.rb before
> the backend is set. For wincairo and other platforms that use cmake, the
> correct file name is set in cmake which causes emitWinAsm to be set
> correctly. DWARF2 annotations will still be incorrectly set for cross
> compiling to a x86_(_64)_win backend. It would be a nice change to have but
> it is much more involved. 
> 
> I agree with removing isGCC, I can make a patch for that. Let me know if you
> think it's still worthwhile to remove isMSVC.

You can just remove isGCC in this patch.  Add a comment in the ChangeLog to say that it is unused.

You should not remove isMSVC.  I think you should update it to use isWin like you did for isIntelSyntax.

You should remove isCompiledForWindows since it will be now unused after the above changes.

[Build Bot]

Comment on attachment 290250 [details]
Patch4

Attachment 290250 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2171242

New failing tests:
fast/images/pixel-crack-image-background-webkit-transform-scale.html

[Build Bot]

Created attachment 290266 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Christopher Reid]

Created attachment 290282 [details]
Work in progress patch

Here's a WIP patch for supporting isMSVC to use isWIN on the asm.rb side. I moved the comment style to be specific to which backend code is being generated for. 

> +    $output.puts("//" + inputHash)
The one problem I had was it's a chicken and an egg problem for which comment style to use for the initial hash comment. Currently, we need to use either ';' or '//' to store the hashes to check to see if we need to re-build. We don't know what backends we're building for until we start parsing the file and we need to put the hash comment before we start generating outputs for each backend. There will also be problems if there's a mix of backends using different comment styles.

Here's some possible solutions for this problem:
- Buffer the output and write afer we've generated code for all backends and customize the comment then
- Switch to a new way to check for changes rather than storing hashes in comments
- Output the comment based on the first backend we use

Do you have any other ideas or input on how I should go about this?

Thanks

[Christopher Reid]

> - Switch to a new way to check for changes rather than storing hashes in
> comments

I could store the hashes in a separate file, would that be okay?

[Konstantin Tokarev]

It seems to me that it could be done without hash at all. When LLIntOffsetsExtractor is generated we could just remove LLIntAssembly.h/LowLevelInterpreterWin.asm if old file is not identical to new.

[Filip Pizlo]

Is there a bot that builds in this configuration hooked up to build.webkit.org?

If not, then I don't think we should take the patch into trunk, since there would be no way for a WebKit developer to validate whether a change to the code you add would be valid for you.  In fact, the new code would be dead code from the standpoint of the WebKit project.

I think it's best for these kinds of changes to be kept downstream until you have a bot on build.webkit.org.

[Don Olmstead]

(In reply to comment #20)
> Is there a bot that builds in this configuration hooked up to
> build.webkit.org?
> 
> If not, then I don't think we should take the patch into trunk, since there
> would be no way for a WebKit developer to validate whether a change to the
> code you add would be valid for you.  In fact, the new code would be dead
> code from the standpoint of the WebKit project.
> 
> I think it's best for these kinds of changes to be kept downstream until you
> have a bot on build.webkit.org.

We do not yet have a build bot going. The porting guide says that before we create the build bot we should have JavaScriptCore building and the tests running. We've been picking off different things that prevent us from building from trunk and this is one of those things. At that point we'll be spinning up a build bot.

Since there are no guarantees that we'll be able to land a port we've been trying to keep our downstream changes to just be CMake files and platform specific code. In our case since we're cross compiling with Clang on Windows this script fails for us. The behavior of the script as is is technically wrong since rather than using the active backend it assumes MASM when Windows is used. So we're attempting a fix here.

We're still new to contributing to WebKit and really appreciate all the help. We'd really like to land a patch here so we can get JSC compiling out of the box. I can see the scope of this is expanding quite a bit but we're more than happy to work through any issues you see with the patch.

[Brent Fulgham]

(In reply to comment #20)
> If not, then I don't think we should take the patch into trunk, since there
> would be no way for a WebKit developer to validate whether a change to the
> code you add would be valid for you.  In fact, the new code would be dead
> code from the standpoint of the WebKit project.

I am in favor of taking this change for the following reasons:
1. Visual Studio now ships with a Clang front-end, which I would like to start using. The work the Sony people are doing helps in this effort, and would save time for me and PerArne.
2. I think the amount of 'dead code' we are talking about there is likely to be small.
3. If we break the Sony build during a period of time that they do not have a buildbot, it's on them to track down and fix the problem.

Therefore, I don't see much downside in allow this work to proceed. If it becomes a problem, we can roll out the changes in the future.

[Filip Pizlo]

(In reply to comment #22)
> (In reply to comment #20)
> > If not, then I don't think we should take the patch into trunk, since there
> > would be no way for a WebKit developer to validate whether a change to the
> > code you add would be valid for you.  In fact, the new code would be dead
> > code from the standpoint of the WebKit project.
> 
> I am in favor of taking this change for the following reasons:
> 1. Visual Studio now ships with a Clang front-end, which I would like to
> start using. The work the Sony people are doing helps in this effort, and
> would save time for me and PerArne.
> 2. I think the amount of 'dead code' we are talking about there is likely to
> be small.
> 3. If we break the Sony build during a period of time that they do not have
> a buildbot, it's on them to track down and fix the problem.
> 
> Therefore, I don't see much downside in allow this work to proceed. If it
> becomes a problem, we can roll out the changes in the future.

OK.

[Brent Fulgham]

Comment on attachment 290250 [details]
Patch4

View in context: https://bugs.webkit.org/attachment.cgi?id=290250&action=review

> Source/JavaScriptCore/offlineasm/x86.rb:141
> +    isWin

I think it might be easier/safer if we simply added a command-line argument we could use to tell 'offlineasm' that we want to use MASM or not-MASM.

Why not just add an optional argument you could use when building that would say "override the assembler to be xxx"?

[Christopher Reid]

Created attachment 290396 [details]
Patch to remove the hash

Here's a patch I made to remove the hash comment for reference. 
It should work okay with cmake as cmake rebuilds based on changes in dependencies. 
I'm not working on mac so I don't know how xcode handles re-generation but it might always regenerate the parse tree with this change.

>Why not just add an optional argument you could use when building that would say "override the assembler to be xxx"?

When I first started looking at this task, I looked in to making this change as a command line option. Using the build rather than the ENV OS check to automatically determine the type of asm felt a lot cleaner to me.
There are a few changes I might have to make when dealing with arguments and I was wondering which approach I should take for them.
Right now the script expects input/output files in a specific order as the first 3 arguments.
I think I would have to either remake those arguments in to --FILE=X args or the new argument would need to always be after those arguments.
The input hash would have to take in to account the new possibility that the output asm can change based on an argument. 
To solve this, I would have to hash the arguments passed. If I make the change that arguments can be in any order, it's a small issue but I think arguments would need to be sorted before hashing since the order of arguments doesn't matter.

Should I continue along the lines of this patch or should I work towards making this a command line change?
I'm not sure which would be the best approach if wincairo is switching to clang. 
Does visual studio's clang use the masm assembler? I agree a command line option might be best if that's the case.

[Mark Lam]

I talked with Christopher offline.  This is how we should proceed:

1. Chris will add an optional --assembler=<assembler name> after the first 3 mandatory args that the offlineasm currently takes.
2. Chris will change isMSVC and isIntelSyntax to check if the optional --assembler is specified and --assembler=MASM.  Else, they will return false.
3. Chris will change the Windows build files that invoke the offlineasm to pass --assembler=MASM.  This will test that this patch worked as intended based on our current buildbot testing for Windows.

For Chris' cross compiling needs which may require build file changes, let's do that in a separate patch when their port is ready to be uploaded.  If anyone think we should approach this differently, please chime in.  Thanks.

[Christopher Reid]

Created attachment 290523 [details]
Command Line Patch

I've added a command line flag to enable the MASM assembler.

[Filip Pizlo]

Comment on attachment 290523 [details]
Command Line Patch

Looks sensible.

[Mark Lam]

Comment on attachment 290523 [details]
Command Line Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290523&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        The funcitons isGCC and isCompilingToWindows were removed as they are no longer called.

typo: /funcitons/functions/

[Christopher Reid]

Created attachment 290531 [details]
Command line patch

Fixing a typo in functions.

[WebKit Commit Bot]

Comment on attachment 290531 [details]
Command line patch

Clearing flags on attachment: 290531

Committed r206759: <http://trac.webkit.org/changeset/206759>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.