clang-format makes various xptcall inline assembly macros hard to read

RESOLVED FIXED in Firefox -esr60

Status

()

defect
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks 1 bug)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 fixed, firefox65 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

6 months ago
The xpcom/reflect/xptcall/md/ has a bunch of different implementations of some of the lowest level of XPCOM call stuff.

Mostly this gets reformatted just fine, but there are a number of macros used to define inline assembly that get reformatted in a way that makes it very hard to understand.

For instance, here's an except of the changes to the STUB_ENTRY macro in md/unix/xptcstubs_arm.cpp:

-#define STUB_ENTRY(n)  \
-nsresult nsXPTCStubBase::Stub##n ()  \
-{ \
-  __asm__ (                            \
-"      mov     ip, #"#n"\n"                                    \
-"      b       SharedStub\n\t");                               \
-  return 0; /* avoid warnings */                                \
-}
+#define STUB_ENTRY(n)                     \
+  nsresult nsXPTCStubBase::Stub##n() {    \
+    __asm__("  mov     ip, #" #n         \
+            "\n"                          \
+            "  b       SharedStub\n\t"); \
+    return 0; /* avoid warnings */        \
+  }

The line "      mov     ip, #"#n"\n", which is a single line of assembly, gets split into multiple lines, which is confusing.

This isn't even the worst instance of it. If you want to see a bad one, check out the output for STUB_MANGLED_ENTRY in xptcstubs_alpha_openbsd.cpp.

There's clearly been a lot of copy pasting between these files. This issue affects many, but not all, of the inline assembly macros.

Files I noticed this issue in are:
  xptcstubs_alpha_openbsd.cpp
  xptcstubs_linux_alpha.cpp
  xptcstubs_arm.cpp
  xptcstubs_arm_openbsd.cpp
  xptcstubs_arm_openbsd.cpp
  xptcstubs_gcc_x86_unix.cpp
  xptcstubs_ppc64_linux.cpp
  xptcstubs_ppc_linux.cpp
  xptcstubs_ppc_openbsd.cpp
  xptcstubs_x86_64_darwin.cpp
  xptcstubs_x86_64_linux.cpp

Also, from the win32 directory:
  win32/xptcstubs.cpp
  win32/xptcstubs_x86_64_gnu.cpp

Obviously not all of these are important, but it does seem like some of them are for Tier 1 platforms.

The right fix would be to go through all of the macros declared in all of these files and disable clang format for the problematic macros. A hackier fix would just be to disable clang format for everything in these directories. It depends on whether somebody wants to dig through all of these files by Friday. This code is quite crusty so I don't think leaving it alone would be that harmful. It seems better than letting these macros get pushed around like that.
Assignee

Updated

6 months ago
Summary: clang-format messes up various xptcall inline assembly macros → clang-format makes various xptcall inline assembly macros hard to read
Assignee

Comment 1

6 months ago
I updated the summary to emphasize that this is about the readability of the output, not the correctness.
Assignee

Comment 2

6 months ago
Macros that I noticed were affected in these various files are STUB_MANGLED_ENTRY, STUB_ENTRY, STUB_HEADER and STUB_SIZE.
(In reply to Andrew McCreight [:mccr8] from comment #0)
> The right fix would be to go through all of the macros declared in all of
> these files and disable clang format for the problematic macros. A hackier
> fix would just be to disable clang format for everything in these
> directories. It depends on whether somebody wants to dig through all of
> these files by Friday. This code is quite crusty so I don't think leaving it
> alone would be that harmful. It seems better than letting these macros get
> pushed around like that.

I am OK with disabling clang-format on these directories, particularly if we can comment as to why the directories at getting ignored in the file that disables them.
Assignee

Comment 4

6 months ago
Sounds like a plan. I'll file a followup bug to use clang-format off because it would be nice for the rest of each file to have a nicer format.
Assignee: nobody → continuation
Assignee

Updated

6 months ago
See Also: → 1510781
Assignee

Comment 6

6 months ago
Many of the inline assembly macros in these files get reformatted
poorly by clang-format, so don't format them for now. In followup
work, in bug 1510781, hopefully we can write a larger patch to only
disable clang-format for the specific macros and then format the rest
of the files.

The problematic include STUB_ENTRY and STUB_MANGLED_ENTRY.
Assignee

Comment 7

6 months ago
This patch disables clang formatting for the two XPTCall stubs directories. I verified locally that when running
  ./mach clang-format -p xpcom/reflect/xptcall/
that these two directories are not formatted.

Comment 8

6 months ago
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c67e1875a652
Disable clang-formatting for XPTCall stub files. r=Ehsan

Comment 9

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c67e1875a652
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Comment on attachment 9028458 [details]
Bug 1510478 - Disable clang-formatting for XPTCall stub files.

[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: 65

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is NPOTB.

String or UUID changes made by this patch: None
Attachment #9028458 - Flags: approval-mozilla-esr60?
Comment on attachment 9028458 [details]
Bug 1510478 - Disable clang-formatting for XPTCall stub files.

OK for uplift to ESR for clang-format project.
Attachment #9028458 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.