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

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

It isn't fixed, please go ahead :)

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

(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.

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.

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

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

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 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.

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

Backout by dvarga@mozilla.com: 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

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

Pushed by npierron@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c549579400ed Prepare the check_macroassembler_style python script to accept clang-format rewritting. r=jandem

(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.

[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 on attachment 9030740 [details] [diff] [review] ESR patch OK for uplift to ESR60 as part of the clang-format project.