1499323 - Update config/check_macroassembler_style.py to handle different code layout

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement, P1)

Description

[Sylvestre Ledru [:Sylvestre]]

In bug 1498618, I ignored the reformatting of MacroAssembler:: code.

Without that, we should be able to handle different code layout.
For example, this simple diff in js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp is failing the check.
-void
-MacroAssembler::PopRegsInMaskIgnore(LiveRegisterSet set, LiveRegisterSet ignore)
-{
+void MacroAssembler::PopRegsInMaskIgnore(LiveRegisterSet set, LiveRegisterSet ignore) {

% python config/check_macroassembler_style.py                     [11:56:01]
--- check_macroassembler_style.py declared syntax
+++ check_macroassembler_style.py found definitions
@@ -1325,10 +1325,9 @@
     is defined in MacroAssembler.cpp
 void PopRegsInMask(LiveRegisterSet);
     is defined in MacroAssembler.cpp
-void PopRegsInMaskIgnore(LiveRegisterSet, LiveRegisterSet) PER_SHARED_ARCH;
-    is defined in arm/MacroAssembler-arm.cpp
-    is defined in arm64/MacroAssembler-arm64.cpp
-    is defined in x86-shared/MacroAssembler-x86-shared.cpp
+void PopRegsInMaskIgnore(LiveRegisterSet, LiveRegisterSet) DEFINED_ON(arm, arm64);
+    is defined in arm/MacroAssembler-arm.cpp
+    is defined in arm64/MacroAssembler-arm64.cpp
 void PopStackPtr() DEFINED_ON(arm, x86_shared);
     is defined in arm/MacroAssembler-arm.cpp
     is defined in x86-shared/MacroAssembler-x86-shared.cpp
TEST-UNEXPECTED-FAIL | check_macroassembler_style.py | actual output does not match expected output;  diff is above

Comment 1

[dishant.vyas93]

Hey, I would like to work on this bug if it's still not solved yet.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

It isn't fixed, please go ahead :)

Comment 3

[dishant.vyas93]

Do I need to create a python file and start from scratch because I can't seem to find that specific file you mentioned?

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to dishant.vyas93 from comment #3)
> Do I need to create a python file and start from scratch because I can't
> seem to find that specific file you mentioned?

The file is located in the config directory of mozilla-central repository:

  https://searchfox.org/mozilla-central/source/config/check_macroassembler_style.py

The goal of this issue is to fix the ""parsing"" rules of C++ functions declarations/definitions such that the output of this script does not change after applying the 4 lines changes from comment 0 to the C++ file.

Comment 5

[Jan de Mooij [:jandem]]

nbp, considering the reformat is going to happen pretty soon, maybe you can take this bug? I'm worried that if we don't fix this, our MacroAssembler code will quickly become an awkward mix of multiple coding styles.

Comment 6

[Nicolas B. Pierron [:nbp]]

Good call, I will see what I can do to get this fixed before Friday.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Not a big deal if it doesn't land by Friday, we can always update it later.

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: Prepare the check_macroassembler_style python script to accept clang-format rewritting.

This patch fixes all the issues reported with check_macroassembler_style.py after removing all clang-format annotations from JIT files and running:

  $ find js/src/jit -name \*.cpp -o -name \*.h | clang-format -i -style=Google

Among the issues we have:
  - Moving the opening brace of method definitions.
    = This is solved by counting open/closed braces in get_macroassembler_Definitions.
  - Adding a new line after the opening braces when overflowing with arguments.
    = This is solved by replacing "(\s+" by "(" in get_normalized_signatures.
  - Removing empty lines after public: and private: keywords.
    = This is solved by resetting the state of lines in get_macroassembler_declaration.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 9028304 [details] [diff] [review]
Prepare the check_macroassembler_style python script to accept clang-format rewritting.

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

Thanks for fixing this. r=me assuming you tested this by introducing some masm issues before/after clang-format to check the script still catches them.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c1893bd77c2
Prepare the check_macroassembler_style python script to accept clang-format rewritting. r=jandem

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09a9f71303cb
Backed out changeset 5c1893bd77c2 for linting failure at /builds/worker/checkouts/gecko/config/check_macroassembler_style.py:149:5 on a CLOSED TREE

Comment 12

[Daniel Varga [:dvarga]]

Failure log: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=5c1893bd77c26d9fc69aa5df4820d76c0ba5100a&selectedJob=214607016

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c549579400ed
Prepare the check_macroassembler_style python script to accept clang-format rewritting. r=jandem

Comment 14

[Nicolas B. Pierron [:nbp]]

(In reply to Daniel Varga [:dvarga] from comment #12)
> Failure log:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&
> revision=5c1893bd77c26d9fc69aa5df4820d76c0ba5100a&selectedJob=214607016

This should now be fixed in comment 13.

Comment 15

[Razvan Maries]

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

Comment 16

[(no longer active)]

Attachment: ESR patch

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is required for easier backporting of patches after the reformatting of ESR using clang-format.

User impact if declined: Declining this will negatively impact our developers' ability to easily backport their patches to ESR in the future.

Fix Landed on Version: 64

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Comment removals only plus a change to a python file which is NPOTB, should not change anything in the binary.  Has also been tested in Nightly for a couple of months or so...

String or UUID changes made by this patch: None

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9030740 [details] [diff] [review]
ESR patch

OK for uplift to ESR60 as part of the clang-format project.

Comment 18

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/d3ef39d3e9e3