Closed Bug 1226188 Opened 4 years ago Closed 4 years ago

"warning C4003: not enough actual parameters for macro 'DEFINED_ON_DISPATCH_RESULT'" in MacroAssembler.h

Categories

(Core :: JavaScript Engine: JIT, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: RyanVM, Assigned: nbp)

References

Details

Attachments

(1 file, 2 obsolete files)

Looks like this was caused by bug 1214508.

js\src\jit/MacroAssembler.h(442) : warning C4003: not enough actual parameters for macro 'DEFINED_ON_DISPATCH_RESULT'
js\src\jit/MacroAssembler.h(511) : warning C4003: not enough actual parameters for macro 'DEFINED_ON_DISPATCH_RESULT'
js\src\jit/MacroAssembler.h(694) : warning C4003: not enough actual parameters for macro 'DEFINED_ON_DISPATCH_RESULT'
js\src\jit/MacroAssembler.h(726) : warning C4003: not enough actual parameters for macro 'DEFINED_ON_DISPATCH_RESULT'
Do you have an idea what this means? I assume this is related to the introduction of Pop(reg1, reg2, reg3, reg4) DEFINED_ON(arm64)
Flags: needinfo?(nicolas.b.pierron)
Yes, so first of all this should not change the behaviour and this should still produce the "= delete" statements, as expected.

The problem is that DEFINED_ON_DISPATCH_RESULT [1] has an argument which is evaluated to nothing by DEFINED_ON_EXPAND_ARCH_RESULTS_3 while we iterate over the list of DEFINE_ON_/ARCH/ .  We cannot use a placeholder here without having to explicitly handled all combinations of DEFINED_ON_RESULT_*.

I suggest we work-around this warning by using __VA_ARGS__ instead of Result in DEFINED_ON_DISPATCH_RESULT expression.

I will make a patch for it.

Note: js\src\jit/MacroAssembler.h(726) : warning C4003  should already exists since months now.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/MacroAssembler.h#154
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8690807 [details] [diff] [review]
Use __VA_ARGS__ to avoid warnings with empty argument list.

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

::: js/src/jit/MacroAssembler.h
@@ -148,5 @@
>  #endif
>  
>  # define DEFINED_ON_RESULT_crash   { MOZ_CRASH(); }
>  # define DEFINED_ON_RESULT_define
>  # define DEFINED_ON_RESULT_        = delete

Might be easier to read if we just predefine every macro to "delete" ?
That way we never have an empty argument list...
I would prefer that a little bit more, but not enough to not give an r+ for this.
Attachment #8690807 - Flags: review?(hv1989) → review+
(In reply to Hannes Verschore [:h4writer] from comment #4)
> >  # define DEFINED_ON_RESULT_crash   { MOZ_CRASH(); }
> >  # define DEFINED_ON_RESULT_define
> >  # define DEFINED_ON_RESULT_        = delete
> 
> Might be easier to read if we just predefine every macro to "delete" ?
> That way we never have an empty argument list...

Note that this would not work, because when the macro is called like:

  DEFINED_ON(x86, x64, arm)

it would be expanded to one of the following(*):

  DEFINED_ON_RESULT_define delete delete // on x86
  DEFINED_ON_RESULT_delete define delete // on x64
  DEFINED_ON_RESULT_delete delete define // on arm
  DEFINED_ON_RESULT_delete delete delete // on others

We need to have no expansion if the architecture is not defined such that the only defined selection is coming from the current architecture.

(*) not exactly, because there is a none architecture added as first argument of DEFINED_ON, but this is a detail of implementation, which does change the value of this argument.
I sent it to try, to double check that this is compiling fine and that no
warning appears anymore.

The problem with the previous patch seems to be that the compiler only
expands macros if they are not under parenthesis anymore, which causes the
macro  "= delete" to be expanded before the contatenation with the macro name.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19d92cb78f0f
Attachment #8691963 - Attachment description: Disable C4003 warning as MacroAssembler macros rely on empty macro expansion. r= → Disable C4003 warning as MacroAssembler macros rely on empty macro expansion.
Flags: needinfo?(nicolas.b.pierron)
Attachment #8691963 - Flags: review?(hv1989)
Attachment #8690807 - Attachment is obsolete: true
Comment on attachment 8691963 [details] [diff] [review]
Disable C4003 warning as MacroAssembler macros rely on empty macro expansion.

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

::: js/src/jit/MacroAssembler.h
@@ +146,5 @@
>  #else
>  # error "Unknown architecture!"
>  #endif
>  
> +#if defined(XP_WIN)

Can you add some description on what this does for posterity.

Something like:

// Temporary disable warning "not enough actual parameters for macro" on windows
Attachment #8691963 - Flags: review?(hv1989) → review+
I think this should work, without having to disable a perfectly correct warning that could (I believe) validly be a compile error, per standard C++.

(For what it's worth, I have no idea where this InterCaps convention for macro arguments came from -- camelCaps is definitely the norm in SpiderMonkey.)
Attachment #8692208 - Flags: review?(nicolas.b.pierron)
Attachment #8692208 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8691963 [details] [diff] [review]
Disable C4003 warning as MacroAssembler macros rely on empty macro expansion.

Marking this patch as obsolete as Waldo's proposal is better, and is not a work-around.
Attachment #8691963 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/9bc366f327ab
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.