Now that we are using the scopes everywhere (bug 1523941) we can hide the register names so that they are not misused, be it accidentally or deliberately. The cheap fix is to just give the registers a new name, say, 'ScratchDoubleReg_' or 'gScratchDoubleReg' or 'ScratchDoubleRegPrivate', adding enough commentary around the definiton to scare people off.

(And I think ScratchDoubleReg and ScratchFloat32Reg can perhaps be removed from the none backend stubs altogether.)

Or we could do what we do for ScratchRegister, which is to ignore the problem and hope that people stick to the rules, and that seems to work mostly well. I only found one unscoped use of ScratchRegister in all of our tier-1 back-ends (in Assembler-arm.cpp).

nbp, jandem - opinions about this?

Hey Lars. I am new to Mozilla - this would be my second open-source contribution. I am keen to help out, but am wondering - is there proper mentorship available to help create a patch and push it? I haven't done much with register names and scopes, so that would help.

Thanks for your time :-)

(Please ignore my last comment- I didn't realise this was in C++, I thought it was in JS :) )

hey Rizwan are you still interested in this bug?

(OOPS! Please scratch/ignore that last comment :) ... let me try this again lol )

Hello Lars,
I'm new to mozilla and this will be my first time contributing, will it be okay if I try to fix this bug?

(In reply to elrichemy from comment #4)

(OOPS! Please scratch/ignore that last comment :) ... let me try this again lol )

Hello Lars,
I'm new to mozilla and this will be my first time contributing, will it be okay if I try to fix this bug?

Hi, and sorry for the long response time, I'll be more attentive going forward. Yes, this bug is available to work on and it would be great if you would do that.

Hey cool. I didn't think I was going to get a response so I forgot about this. I'm glad I decided to check it one more time just for the hell of it though

I'm going to be honest, I've tried to understand and google the problem and the solutions provided in the comments but I still can't put two and two together. I'm confused, I don't know where to start asking questions but I'm going to try anyway.

"The cheap fix is to just give the registers a new name, say, 'ScratchDoubleReg_' or 'gScratchDoubleReg' or 'ScratchDoubleRegPrivate', adding enough commentary around the definition to scare people off." What are register names?

"ScratchDoubleReg and ScratchFloat32Reg can perhaps be removed from the none backend stubs altogether." what are none backend stubs?

I've tried googling this with no success, do you happen to know where I can find more information about register names and none backend stubs?

Hello
I'm still struggling with finding these register names. I'm sure its simple, any guidance will be appreciated.

Hi, thanks for getting back in touch.

Google will not help you here, you have to look at the source code. If you go to searchfox.org it will allow you to search the Firefox source code. If you type in eg ScratchDoubleReg into the search field it will show you definitions and uses of that name. In particular, it is defined by the ARM and x64 assemblers and used by those to define the ScratchDoubleScope assertion. If you change the name of ScratchDoubleReg to eg ScratchDoubleReg_ and change those uses (which are legitimate) to match, and recompile for those platforms, then the code should still compile and run properly. The point is simply that the new name flags that it should not be referenced freely, and we would want to add a comment at the definition saying so.

If you scroll down on that search page, then you'll see a lot of occurrences in the MIPS back-ends. Don't worry about those; we don't maintain that code ourselves. But if you look carefully there are also occurrences in MacroAssembler-none.h and Assembler-x86.h. The reason these are not listed above with "definitions" and "uses" is just that searchfox does not index all platform assemblers. But they need to be fixed in the same way even so. The one in MacroAssembler-none.h can possibly just be removed. The other one needs a name change.

Finally there's a use in WasmBaselineCompiler.h. This will need to be adapted too; initially it can just be renamed to the new name.

Ditto for ScratchFloat32Register.

I suggest that you start with the x64 back-end because it's easier to deal with, and ignore the others initially; it'll take some work on your part to set up the compilation system anyway, and that's easiest on that platform.

Awesome Thank you... I'll stay touch

Hey I made some changes and wanted to show a screenshot of it but it doesn't look like I can upload it here. Here is what I have done so far.

It took me awhile to figure out if I had the right source code for this, I hope I do. I added the underscore to all ScratchDoubleReg's in these files:
Assembler-arm.h
Assembler-x64.h
Assembler-x86-shared.h
MacroAssembler-none.h
Assembler-x86.h

I also couldn't find WasmBaselineCompiler.h in searchfox.org so I'm not sure if its WasmBaselineCompile.cpp or WasmBaselineCompile.h?

(In reply to nalanirojas25 from comment #10)

Hey I made some changes and wanted to show a screenshot of it but it doesn't look like I can upload it here. Here is what I have done so far.

It took me awhile to figure out if I had the right source code for this, I hope I do. I added the underscore to all ScratchDoubleReg's in these files:
Assembler-arm.h
Assembler-x64.h
Assembler-x86-shared.h
MacroAssembler-none.h
Assembler-x86.h

The way we discuss changes is usually by creating a patch file and discussing that. I don't know if you've checked out the Firefox source code using Mercurial or git; but the command to generate a patch is usually hg diff or git diff, this will show all the changes in your working directory. You should be able to capture the output of that in a file and upload it to this bug using the "attach new file" button above, and then we can discuss it.

I also couldn't find WasmBaselineCompiler.h in searchfox.org so I'm not sure if its WasmBaselineCompile.cpp or WasmBaselineCompile.h?

I am sorry, I mistyped. It is WasmBaselineCompile.cpp.

cool, I'll work on creating the patch. I found a link ( https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch ) to learn how to do it but let me know if you have of a better guide for it. Other than that, I'll let you know if I get stuck and/or when I'm done. thanks

Here are the output changes for ScratchDoubleReg.

One thing I didn't do was add a comment because I'm not 100% clear what the comment should inform. Do you want to just let people know that the name had to be changed for reference reason in searchfox.org? or reference reason within the code?

(In reply to nalanirojas25 from comment #12)

cool, I'll work on creating the patch. I found a link ( https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch ) to learn how to do it but let me know if you have of a better guide for it. Other than that, I'll let you know if I get stuck and/or when I'm done. thanks

That guide should be fine.

A patch is never a png screenshot though, it is the diff itself, the text files that you have taken a screenshot of above. Can you upload these instead?

How do I do that? I can't find clear detailed instructions and I'm just going in circles.

I found a guide but I'm running into technical problems. Here is the guide I found (https://mozilla-l10n.github.io/documentation/tools/mercurial/creating_mercurial_patch.html) When I get to the "commit changes" , i get this error:

0:01.29 ERROR: You're using an old version of clang-format binary. Please update to a more recent one by running: './mach bootstrap'

So I ran: "./mach bootstrap" and I get another error:

stable-x86_64-pc-windows-msvc updated - rustc 1.41.1 (f3e1a954d 2020-02-24)

error: could not remove 'rustup-bin' file: 'C:\Users\User.cargo\bin\rustup.exe'
error: caused by: Access is denied. (os error 5)
error: toolchain 'stable-x86_64-pc-windows-msvc' does not contain component 'rustfmt' for target 'x86_64-pc-windows-msvc'
Error running mach:

['bootstrap']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.
You can invoke |./mach busted| to check if this issue is already on file. If it
isn't, please use |./mach busted file| to report it. If |./mach busted| is
misbehaving, you can also inspect the dependencies of bug 1543241.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:
subprocess.CalledProcessError: Command '['c:/Users/User\.cargo\bin\rustup.exe', 'component', 'add', 'rustfmt']' returned non-zero exit status 1.

Can I get some help on understanding what this means please?

It looks like it was able to update some files but it couldn't remove others. So if that's the case, I would like to know what is the best approach to fixing this but I might also be misinterpreting it as well. Please help lol

Thanks for your patience. Things are not always easy :-)

First, about the patch - you don't need to do any of those things to create a diff that you can upload to this bug and that we can discuss. (It's true that you're supposed to do all those things, but we don't have to start there.) With mercurial, you just do 'hg diff > bug1524481.patch' from within the mozilla-central source directory, and then you can attach the file 'bug1524481.patch' to this ticket and we can take it from there. (This all assumes you've not actually committed the changes yet with hg commit; in that case the command needs to diff against the head of the repository instead: 'hg diff -r central > bug1524481.patch' should work.)

Second, about the problems you're having. It looks like you don't have permissions to modify some files because mach bootstrap is trying to write some files to /Users/User.cargo/, which is not your home directory. I don't use windows myself, so I'm not sure what to tell you. Did you follow the Windows instructions here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites?

Hey,
I couldn't verify my last account and I had to create a new. I tried for awhile to recover it cause I really didn't want to start another one but there wasn't a way around it. I hope this doesn't cause any problems.

Thanks, I was able to fix the error. Sorry its taking me so long to figure this out. I'm still working on it though.

Please take a look at this file instead.

About the comment that should be included, did you want to just let people know that the name had to be changed for reference reason in searchfox.org? or reference reason within the code? and in what file should I place the comment in?

OK, this is starting to look good!

What I had in mind for a comment was just a one-liner above the definition in each Assembler file that says something like "Normally you do not want to reference this directly, you want to use ScratchDoubleScope instead."

Is there anything else that needs to be address with regarding the ScratchDoubleReg_? I was planning on moving on and
to doing the same thing with ScratchFloat32Register if all is good.

(In reply to Nala from comment #27)

Is there anything else that needs to be address with regarding the ScratchDoubleReg_? I was planning on moving on and
to doing the same thing with ScratchFloat32Register if all is good.

Not at first blush, anyway, so I suggest you move forward as you propose. Once you think the patch is complete we'll move on to the next phase, which is to get it properly reviewed and tested.

Sounds good.

Should I add this comment "Do not reference ScratchFloat32Reg_ directly, use ScratchFloat32Scope instead." for ScratchFloat32Reg as well?

Sorry! I forgot to make one more change in one more file. Please take a look at this file instead. thanks

Also, should ScratchDoubleReg_ and ScratchFloat32Reg still be removed from the backend stubs? if so what are those files.

(In reply to Nala from comment #29)

Should I add this comment "Do not reference ScratchFloat32Reg_ directly, use ScratchFloat32Scope instead." for ScratchFloat32Reg as well?

Yes, that seems appropriate. Patch looks good.

Also, should ScratchDoubleReg_ and ScratchFloat32Reg still be removed from the backend stubs? if so what are those files.

They should be removed (if possible) from the "none" backend stubs; in this context, "none" is our architecture that has no JIT. So these files are in js/src/jit/none.

It's hard to say whether it's possible to remove those definitions without test-compiling, which is why the initial description says "if possible". Hopefully you're set up to test compile by now, so it's possible for you to try that out. If you're just building spidermonkey and not all of firefox you want to reconfigure your build directory with --disable-ion and then rebuild spidermonkey.

After looking into all the files in the 'none' folder, I only saw one file that has ScratchDoubleReg and ScratchFloat32Reg being set. That file is MacroAssember-none.h. I removed lines 23-25, built it, and ran it but I'm not sure where to look for any possible problems so I was hoping you can help me out on that. I did "./mach build" to build firefox, and "./mach run" to run it. Where do I look and what do I look for when running this test? I'm not sure how to confirm whether there was a problem or not.

Right. The way we usually do this is just build the JS shell by itself and (if necessary) run tests in the shell. Here's what I do if I want to build a shell for the "none" target. Starting out in the top-level directory of the mozilla-central checkout:

cd js/src
mkdir build-debug
cd build-debug
../configure --enable-debug --disable-optimize --disable-ion
make -j16

If you get failures during compilation then your change was wrong, otherwise it was fine. No more testing than that is needed in this case, because all you did was remove some definitions that we thought were unused.

For future reference:

Now if you just run (in the same directory) dist/bin/js you should get a prompt that looks like js>, this is the JS shell. Type quit() to exit.

You can run test cases here. For example, you can run ../jit-test/jit_test.py dist/bin/js atomics to run the regression tests for the JS atomic operations. (Or you can run all of them by leaving off the last argument to that command but that can take a little time.)

gotcha, I ran the commands and everything looks good. Thanks for being so patient and helpful! I really appreciated it.

I'm not sure what happens after this. What else needs to be done?

(Sorry for the lag, very busy at present.)

So now begins the fun. You have a patch and you've tested it, but you need to get it reviewed before it can be landed in mozilla-central. For this, you need to upload the patch to our patch-review system, known as phabricator / moz-phab. Documentation for that is here: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html. (Basically, you just need to create an account and install+setup moz-phab.) After that, you commit your patch locally in your source tree with a commit message that reads "Bug 1524481 - (your brief explanation here). r?lth" and then you run "moz-phab submit", which will upload the patch to phabricator. I will then receive an email that I have a new review request, and I can then review the patch properly and approve it or ask for changes.

If you get stuck, you can ask questions here but you can also use our chat system at chat.mozilla.org; if you ask questions in the room "spidermonkey" there will be people there who can help you out if you're stuck and don't want to wait for me to notice that you have a question.

(In reply to Lars T Hansen [:lth] from comment #0)

Now that we are using the scopes everywhere (bug 1523941) we can hide the register names so that they are not misused, be it accidentally or deliberately. The cheap fix is to just give the registers a new name, say, 'ScratchDoubleReg_' or 'gScratchDoubleReg' or 'ScratchDoubleRegPrivate', adding enough commentary around the definiton to scare people off.

As long as this is no longer some implicit knowledge, this would be fine for me.

And if these value still have to be reachable, one way would be to make them private fields of the class which needs to access these values.

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Hi! I am Garima, an Outreachy participant, and I would like to work on this bug :)

Hi Garima, I'm happy for you to work on this. As you see, there's quite a bit of work that's been done already but it was never completed. You should probably start by reading the conversation above and then let me know about any further guidance that you may need. I'm happy to answer all questions here or on Matrix (I'm on European time and I don't usually work in the evenings).

Hi! Before starting off, I just wanted to ask that while submitting patches, should I do it using "moz-phab submit", or the way it's been done above? Also I searched up ScratchDoubleReg_ in searchfox and did not find anything, so will I have to go over everything Nala has done above?

moz-phab submit is fine, it's what we use now mostly. You can create a new patch if you prefer, or you may be able to "commandeer" the existing patch from the phabricator user interface. Do whatever's easiest for you. Also, as a general rule, we prefer for you to reuse/reboot existing work when that is available, but since the point here is also for you to learn things I'm not going to object if you choose to start from scratch.

As far as I know, none of the work Nala did ever landed in mozilla-central, so there are no instances of ScratchDoubleReg_ in the source - the point of the patch is to rename the definitions of ScratchDoubleReg as ScratchDoubleReg_ (etc) and fix the code that breaks as a consequence of that. Only ScratchDoubleScope (etc) should be allowed to reference ScratchDoubleReg_, other instances of ScratchDoubleReg will have to be rewritten using ScratchDoubleScope.

The one in MacroAssembler-none.h can possibly just be removed. The other one needs a name change.

So I was making the required changes and going through the previous patches where I saw that even in MacroAssembler-none.h, Nala had made the name changes and added comments, but in your initial message you've mentioned that they can be removed, so I'm confused if I should change the name or remove all lines that reference ScratchDoubleReg and ScratchFloat32Reg.

Also had a doubt about running tests, should I just run ./mach jit-test and ./mach build and see if they're successful? Or are there any other tests I should run?

As for testing, it should be sufficient to build the JS shell by itself and then run mach jit-test (or, equivalently, run jit-tests directly). Generally speaking you need to do this on all platforms (x86, x86_64, arm32, arm64, none) and both with and without DEBUG, lmk if you need help with setting up build configurations for any of those, it's not hard. Unfortunately the try server only covers a few of those combinations, so for wide-ranging changes like these I end up running a lot of tests manually.

I believe that the definitions of ScratchDoubleReg and ScratchFloat32Reg can just be removed on the none platform. I could be wrong :-) but it would be a little strange if they were referenced anywhere. In your place I would try simply to remove them and see what happens when you try to build.

So I'd already built SpiderMonkey, so I just ran "./mach build" and "./mach jit-test" and they've both been successful.

Generally speaking you need to do this on all platforms (x86, x86_64, arm32, arm64, none) and both with and without DEBUG, lmk if you need help with setting up build configurations for any of those, it's not hard. Unfortunately the try server only covers a few of those combinations, so for wide-ranging changes like these I end up running a lot of tests manually.
However I didn't quite understand what you meant by these lines, am I supposed to build something more?
Also, please let me know what I should do next :)

So I'd already built SpiderMonkey, so I just ran "./mach build" and "./mach jit-test" and they've both been successful.
However I didn't quite understand what you meant by these lines, am I supposed to build something more?

As for testing, it should be sufficient to build the JS shell by itself and then run mach jit-test (or, equivalently, run jit-tests directly). Generally speaking you need to do this on all platforms (x86, x86_64, arm32, arm64, none) and both with and without DEBUG, lmk if you need help with setting up build configurations for any of those, it's not hard. Unfortunately the try server only covers a few of those combinations, so for wide-ranging changes like these I end up running a lot of tests manually.

Please ignore the above comment, I don't quite understand the markdown styling yet x')
Also, please let me know what I should do next :)

When you run ./mach jit-test you are running the tests on a single configuration of the shell, the one you've built - usually this is for the x86_64 architecture and either with debug checks present or not (depending on how you built the shell).

But a change like this that touches all architectures requires you to at least test-compile for all architectures, and both with and without debug checks present, because you need to test whether you've changed the source correctly everywhere. (And ideally also run jit-tests for each configuration.)

I never build the shell using ./mach build, but instead I do things like this to build with debug checks for arm64:

cd js/src
mkdir build-arm64-debug
cd build-arm64-debug
../configure --enable-debug --enable-simulator=arm64
make -j16

There are nine combinations to test: four platforms {x86_64, x86, arm, arm64} with two configurations each {debug, non-debug} plus the none platform (in either configuration).

To compile with debug checks, you configure with --enable-debug, the default is no debug checks.

To select arm and arm64, you configure with --enable-simulator={arm,arm64}

To select x86 (32-bit) you configure with --target=i686-pc-linux (at least if you're on Linux; if you're on other platforms I don't know)

To select the "none" platforms you configure with --disable-jit.

I followed the above steps but when I run "../configure --enable-debug --enable-simulator=arm64" I get "../configure: No such file or directory", so what have I done wrong?

Hm, that's unfortunate, this seems to be some sort of recent change to how the source tree is set up, I've also run into this. I'll attach the configure script, you can just save it in js/src.

(Just a shell script, save it + make it executable and you should be good to go.)

So I just tried running the tests, but when I run "make -j16" I get an error saying MSYS make is not supported :/

Oof, on windows? I think you have to use 'mozmake' instead of 'make'.

I'm not sure what these errors mean, could you please explain if I've done something wrong?

You need to scroll back up in the terminal to find out what is actually not compiling right - what we see here is just the end of the compilation where it says "something went wrong".

So I fixed the issue which I'd gotten the previous time, however when I ran the test again I still get some errors, but for different files, and mostly something like "vixl - unsigned int"

(In reply to Garima Mazumdar from comment #58)

So I fixed the issue which I'd gotten the previous time, however when I ran the test again I still get some errors, but for different files, and mostly something like "vixl - unsigned int"

Vixl is our imported arm64 assembler (js/src/jit/arm64/vixl/). If these new problems are errors then that's interesting; if they are warnings we should investigate, but sometimes we can live with them. Can you post the message including the name of the file and line number this appeared in?

So I've seen this many times for different cases (case SVC, case BL, case CBC etc.) but I'm not entirely sure they're errors?

So I've run the tests for arm, arm64 and none files, and I still get the same error (Error 2) but they do not point to any of the files I've worked on, so do you think I should proceed forward? Also, I was unsure of the commands for x86_64 and x86 so I didn't run the tests for it.

Yeah, you can proceed for now with submitting for review etc, we can get back to this later. The bug above is known; it's weird that it pops up again because I thought it had been resolved. This could be a matter related to the version of the compiler you're using, or something else fairly random. I'll see if I can dig up the bug report later today and look for a solution.

To test for x86_64 you don't normally need any specific configuration changes at all, that's the default for most of the developers, especially on Windows.

To test for x86, on linux I configure with --target=i686-pc-linux, but as you're on Windows the target triple is probably something else, I'll have to look into it. (And there's a small chance the compiler you're using does not know how to target 32-bit x86.) Don't worry about it for now.

Hi Lars, just wanted to know how I should proceed with the bug?

Reviews take time, frequently several days (we usually have a long queue). With a little luck I will get to your patch today.

(In reply to Garima Mazumdar from comment #64)

Hi Lars, just wanted to know how I should proceed with the bug?

r+, so now we need to land it. i guess that's my responsibility, assuming you don't have that level of privileges. i'll mark it for landing and then we'll see.

https://hg.mozilla.org/mozilla-central/rev/0d089ca896ba