Closed Bug 1499323 Opened Last year Closed 11 months ago

Update config/check_macroassembler_style.py to handle different code layout

Categories

(Firefox Build System :: Source Code Analysis, enhancement, P1)

enhancement

Tracking

(firefox-esr60 fixed, firefox65 fixed)

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 --- fixed
firefox65 --- fixed

People

(Reporter: Sylvestre, Assigned: nbp, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][lang=python])

Attachments

(2 files)

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
Mentor: sledru, nicolas.b.pierron
Keywords: good-first-bug
Whiteboard: [good first bug][lang=python]
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.
Severity: normal → enhancement
Priority: -- → P5
Blocks: 1508062
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.
Flags: needinfo?(nicolas.b.pierron)
Good call, I will see what I can do to get this fixed before Friday.
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Flags: needinfo?(nicolas.b.pierron)
Priority: P5 → P1
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.
Attachment #9028304 - Flags: review?(jdemooij)
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.
Attachment #9028304 - Flags: review?(jdemooij) → review+
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
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.
Flags: needinfo?(nicolas.b.pierron)
https://hg.mozilla.org/mozilla-central/rev/c549579400ed
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Attached patch ESR patchSplinter Review
[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
Attachment #9030740 - Flags: approval-mozilla-esr60?
Attachment #9030740 - Attachment is patch: true
Attachment #9030740 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 9030740 [details] [diff] [review]
ESR patch

OK for uplift to ESR60 as part of the clang-format project.
Attachment #9030740 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Keywords: good-first-bug
Target Milestone: Firefox 65 → mozilla65
You need to log in before you can comment on or make changes to this bug.