Closed Bug 1178772 Opened 4 years ago Closed 4 years ago

Add a style-checker for MacroAssembler functions.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

As commented in Bug 1121613 comment 1, we have functions annotations which are used to indicate where functions are located.  These annotations are used to ensure that a reader of this file can locate all the implementation of a function without opening 40 files (*).

We should add a script which ensure that these annotations are maintained properly, and fail to build if any of these annotations does not match the location of the functions.

(*) find js/src/jit -name \*Assembler\* | wc -l
This patch replace the current ONLY_* macro used in the MacroAssembler by a
DEFINED_ON macro which is used by the MacroAssembler in the same way.

The difference being that the DEFINED_ON macro is given a list of
architecture (arm, arm64, mips, x86, x86_shared, x64, none) which is used to
select how the MacroAssembler functions are annotated.

The python script added with this patch looks for signatures within the
check_macroassembler_style sections of the code, and re-construct the
declaration of the methods before comparing them.  This script is mostly
useful to catch inconsistency in the method declarations, missing definition
for some architectures, or typo in the signature name.
Attachment #8628404 - Flags: review?(hv1989)
Comment on attachment 8628404 [details] [diff] [review]
Add check_macroassembler_style.py: Verify that each MacroAssembler declaration maps to all its definitions.

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

I've checked the output of "check_macroassemblyer_style.py" and it is quite hard to understand what should get solved. Can you make it more obvious as to how one can fix the seen issue. E.g. check_spidermonkey_style explains what should get adjusted or which source file should get included above another ...

::: config/check_macroassembler_style.py
@@ +7,5 @@
> +# This script checks that SpiderMonkey MacroAssembler methods are properly
> +# annotated.
> +#
> +# The MacroAssembler has one interface for all platforms, but it might have
> +# one definition per platform. The code of the MacroAssembler use some macro

use a macro

@@ +8,5 @@
> +# annotated.
> +#
> +# The MacroAssembler has one interface for all platforms, but it might have
> +# one definition per platform. The code of the MacroAssembler use some macro
> +# to annoate the method declarations, in order to delete the function if it

to annotate

@@ +38,5 @@
> +            return all_filenames
> +        except:
> +            continue
> +    else:
> +        raise Exception('failed to run any of the repo manifest commands', cmds)

Maybe put this into an util.py file which this and check_spidermonkey_style.py can use?

@@ +129,5 @@
> +        for line in f:
> +            if '//{{{ check_macroassembler_style' in line:
> +                style_section = True
> +            elif '//}}} check_macroassembler_style' in line:
> +                style_section = False

It is the intention to remove this once everything is finished, right?

::: js/src/jit/MacroAssembler.h
@@ +119,5 @@
> +
> +// These macros are short-cuts over the DEFINED_ON macro to avoid listing all
> +// architectures at each function declaration.
> +# define PER_ARCH DEFINED_ON(ALL_ARCH)
> +# define PER_SHARED_ARCH DEFINED_ON(ALL_SHARED_ARCH)

This seems like a huge chunk of code before the start of anything meaningful. Can we make this smaller?
- Would it be possible to combine some of the helper functions?
- Would it be possible to combine all comments into one comment instead of all the comments in between. I think that might be smaller
- Use descriptive names, instead of helper1/2/3, which might also remove the need for some comments.
Attachment #8628404 - Flags: review?(hv1989)
(In reply to Hannes Verschore [:h4writer] from comment #2)
> I've checked the output of "check_macroassemblyer_style.py" and it is quite
> hard to understand what should get solved. Can you make it more obvious as
> to how one can fix the seen issue. E.g. check_spidermonkey_style explains
> what should get adjusted or which source file should get included above
> another ...

Do you have any suggestions?  I found the diff output of check_macroassembler_style to be quite self-explaining, as you can "almost" copy-&-paste the code in the Macro Assembler to get the something compiling. I think the most common case, would be people forgetting to set the proper annotation in the macro assembler header.

> @@ +129,5 @@
> > +        for line in f:
> > +            if '//{{{ check_macroassembler_style' in line:
> > +                style_section = True
> > +            elif '//}}} check_macroassembler_style' in line:
> > +                style_section = False
> 
> It is the intention to remove this once everything is finished, right?

Ideally yes, but as opposed to the macro assembler changes, I am less confident that we would want to parse the full spectrum of C++ grammar in python.  So, I would expect us to keep the one added in this patch for a while, but no more than that (except for new architectures).

> ::: js/src/jit/MacroAssembler.h
> @@ +119,5 @@
> > +
> > +// These macros are short-cuts over the DEFINED_ON macro to avoid listing all
> > +// architectures at each function declaration.
> > +# define PER_ARCH DEFINED_ON(ALL_ARCH)
> > +# define PER_SHARED_ARCH DEFINED_ON(ALL_SHARED_ARCH)
> 
> - Would it be possible to combine some of the helper functions?

Unfortunately no, we need these helper macros to enforce some order on the expansion logic.  The order of the expansion is needed for doing the dispatch to the per-platform macro, otherwise we would have some weird generated token such as   DEFINED_ON_RESULT_DEFINED_ON_none.
Add check_macroassembler_style.py: Verify that each MacroAssembler declaration maps to all its definitions. r=

I modified check_macroassembler_style.py to add extra lines to indicate
where the definitions to expected to be.

Note that, as opposed to check_spidermonkey_style, there is no one truth,
thus either the declaration is correct, or the definition, or none.  But in
any case, they should be synchronized again.

--- check_macroassembler_style.py declaration output
+++ check_macroassembler_style.py definition output
@@ -75,10 +75,9 @@
     is defined on MacroAssembler-inl.h
 inline void PushRegsInMask(LiveGeneralRegisterSet);
     is defined on MacroAssembler-inl.h
-inline void PushRegsInMask(LiveRegisterSet) PER_ARCH;
+inline void PushRegsInMask(LiveRegisterSet) PER_SHARED_ARCH;
+    is defined on x86-shared/MacroAssembler-x86-shared-inl.h
     is defined on mips/MacroAssembler-mips-inl.h
-    is defined on x86/MacroAssembler-x86-inl.h
-    is defined on x64/MacroAssembler-x64-inl.h
     is defined on arm/MacroAssembler-arm-inl.h
     is defined on arm64/MacroAssembler-arm64-inl.h
 inline void PushValue(const Address&);


I modified the comments on the Macros, in order to explicit what is going
on, in one big comment instead of multiple one.  I made 2 comments, one
which explains mostly why we have these and what they are used for, and the
second details the implementation, and explicit how the evaluation proceeds.
The helper macros are renamed to explicit what they are made for.
Attachment #8628404 - Attachment is obsolete: true
Attachment #8629991 - Flags: review?(hv1989)
Comment on attachment 8629991 [details] [diff] [review]
Attachment to Bug 1178772 - Add a style-checker for MacroAssembler functions.

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

::: js/src/jit/MacroAssembler.h
@@ +142,5 @@
> +# define DEFINED_ON_DISPATCH_RESULT(Result)     \
> +    DEFINED_ON_RESULT_ ## Result
> +
> +# define DEFINED_ON_EXPAND_ARCH_RESULTS(ParenResult)    \
> +    DEFINED_ON_HELPER3 ParenResult

self-nit: s/DEFINED_ON_HELPER3/DEFINED_ON_DISPATCH_RESULT/
Comment on attachment 8629991 [details] [diff] [review]
Attachment to Bug 1178772 - Add a style-checker for MacroAssembler functions.

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

::: js/src/jit/MacroAssembler.h
@@ +142,5 @@
> +# define DEFINED_ON_DISPATCH_RESULT(Result)     \
> +    DEFINED_ON_RESULT_ ## Result
> +
> +# define DEFINED_ON_EXPAND_ARCH_RESULTS(ParenResult)    \
> +    DEFINED_ON_HELPER3 ParenResult

To make this macro works well on Windows, we have to expand the argument multiple times to get rid of all the MOZ_FOR_EACH internal macros, and get the final result.  So the code above is modified to:

# define DEFINED_ON_EXPAND_ARCH_RESULTS_3(ParenResult)  \
    DEFINED_ON_DISPATCH_RESULT ParenResult
# define DEFINED_ON_EXPAND_ARCH_RESULTS_2(ParenResult)  \
    DEFINED_ON_EXPAND_ARCH_RESULTS_3 (ParenResult)
# define DEFINED_ON_EXPAND_ARCH_RESULTS(ParenResult)    \
    DEFINED_ON_EXPAND_ARCH_RESULTS_2 (ParenResult)

Try runs:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f8f563fd34d (1 level)
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=63c2ef2e78ed (2 levels)
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcf09474ef00 (3 levels)

Adding extra levels does not change the evaluation of gcc, nor clang.
Comment on attachment 8629991 [details] [diff] [review]
Attachment to Bug 1178772 - Add a style-checker for MacroAssembler functions.

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

Thanks for addressing the issues. Looks much better to me.

::: config/check_macroassembler_style.py
@@ +58,5 @@
> +        signature = re.sub(r'\s+PER_ARCH', '', signature)
> +
> +    elif 'PER_SHARED_ARCH' in signature:
> +        archs = all_shared_architecture_names
> +        signature = re.sub(r'\s+PER_SHARED_ARCH', '', signature)

else: 
 assert raise exception or return None?

@@ +193,5 @@
> +        archs = set(signatures[s])
> +        if len(archs.symmetric_difference(architecture_independent)) == 0:
> +            output.append(s + ';\n')
> +            if s.startswith('inline'):
> +                output.append('    is defined on MacroAssembler-inl.h\n')

s/on/in/

@@ +195,5 @@
> +            output.append(s + ';\n')
> +            if s.startswith('inline'):
> +                output.append('    is defined on MacroAssembler-inl.h\n')
> +            else:
> +                output.append('    is defined on MacroAssembler.cpp\n')

s/on/in/

@@ +207,5 @@
> +            for a in archs:
> +                a = a.replace('_', '-')
> +                masm = '%s/MacroAssembler-%s' % (a, a)
> +                if s.startswith('inline'):
> +                    output.append('    is defined on %s-inl.h\n' % masm)

s/on/in/

@@ +209,5 @@
> +                masm = '%s/MacroAssembler-%s' % (a, a)
> +                if s.startswith('inline'):
> +                    output.append('    is defined on %s-inl.h\n' % masm)
> +                else:
> +                    output.append('    is defined on %s.cpp\n' % masm)

s/on/in/

@@ +234,5 @@
> +
> +    # Compare declarations and definitions output.
> +    difflines = difflib.unified_diff(generate_file_content(decls),
> +                                     generate_file_content(defs),
> +                                     fromfile='check_macroassembler_style.py declaration output',

check_macroassembler_style.py declared syntax

@@ +235,5 @@
> +    # Compare declarations and definitions output.
> +    difflines = difflib.unified_diff(generate_file_content(decls),
> +                                     generate_file_content(defs),
> +                                     fromfile='check_macroassembler_style.py declaration output',
> +                                     tofile='check_macroassembler_style.py definition output')

check_macroassembler_style.py found definitions

::: js/src/jit/MacroAssembler.h
@@ +82,5 @@
> +// is expanded to
> +//
> +//   DEFINED_ON_none DEFINED_ON_arm DEFINED_ON_x86_shared
> +//
> +// which are later expanded by DEFINED_ON_EXPAND_ARCH_RESULTS to either

// which are later expanded on ARM, x86, x64, mips by DEFINED_ON_EXPAND_ARCH_RESULTS to:

@@ +86,5 @@
> +// which are later expanded by DEFINED_ON_EXPAND_ARCH_RESULTS to either
> +//
> +//   define
> +//
> +// if we are on ARM, x86 or x64, or expanded to

// or if the JIT is disabled or set to no architecture to:
Attachment #8629991 - Flags: review?(hv1989) → review+
Assignee: nobody → nicolas.b.pierron
https://hg.mozilla.org/mozilla-central/rev/31435f8615d7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1215211
You need to log in before you can comment on or make changes to this bug.