IonMonkey: MIPS: The DEFINED_ON macro architecture 'mips' definition conffict with GCC builtin macro

RESOLVED FIXED in Firefox 43

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: hev, Assigned: hev)

Tracking

Trunk
mozilla43
Other
Linux
Points:
---

Firefox Tracking Flags

(firefox42 affected, firefox43 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
In js/src/jit/MacroAssembler.h, The ALL_ARCH and ALL_SHARED_ARCH defined and used by the DEFINED_ON macro. This symbol 'mips' is already builtin defined by GCC and value is 1. So, build failed for mips (32-bit).
(Assignee)

Comment 1

3 years ago
Created attachment 8632699 [details] [diff] [review]
bug1182936.patch

This is my workaround, I think that is not a good solution. Do you have some good ways?
(In reply to Heiher from comment #1)
> Created attachment 8632699 [details] [diff] [review]
> bug1182936.patch
> 
> This is my workaround, I think that is not a good solution. Do you have some
> good ways?

hum … I am not sure this is a good thing to make it undefined without re-defining it.

Another solution might be to have it named mips32?  but this might imply that we either move the files or update the check_macroassembler_style.py script in top-level config directory.
(Assignee)

Comment 3

3 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> (In reply to Heiher from comment #1)
> > Created attachment 8632699 [details] [diff] [review]
> > bug1182936.patch
> > 
> > This is my workaround, I think that is not a good solution. Do you have some
> > good ways?
> 
> hum … I am not sure this is a good thing to make it undefined without
> re-defining it.
> 
> Another solution might be to have it named mips32?  but this might imply
> that we either move the files or update the check_macroassembler_style.py
> script in top-level config directory.

Good solution, Can I try it? I will rename mips directory and all symbols in mips-specific files. Looks this is a big changes, do it step-by-step or write at one stretch?
(In reply to Heiher from comment #3)
> (In reply to Nicolas B. Pierron [:nbp] from comment #2)
> > Another solution might be to have it named mips32?  but this might imply
> > that we either move the files or update the check_macroassembler_style.py
> > script in top-level config directory.
> 
> Good solution, Can I try it? I will rename mips directory and all symbols in
> mips-specific files. Looks this is a big changes, do it step-by-step or
> write at one stretch?

Rankov, what do you think?
Flags: needinfo?(branislav.rankov)
I think that renaming a folder over an issue like this seems like an overkill. I would suggest to add prefix to architecture constants. Something like this:

# define ALL_ARCH arch_mips, arch_arm, arch_arm64, arch_x86, arch_x64

Short defines like mips, arm, x86 look like a hazard waiting to happen. In case of mips it has already happened.

You can easily strip "arch_" part in the check_macroassembler_style.py

PS: If we want to merge mips32 and mips64 ports, as discussed in bug 1140954, we shouldn't rename current folder to mips32.
Flags: needinfo?(branislav.rankov)
(Assignee)

Comment 6

3 years ago
(In reply to Branislav Rankov [:rankov] from comment #5)
> I think that renaming a folder over an issue like this seems like an
> overkill. I would suggest to add prefix to architecture constants. Something
> like this:
> 
> # define ALL_ARCH arch_mips, arch_arm, arch_arm64, arch_x86, arch_x64
> 
> Short defines like mips, arm, x86 look like a hazard waiting to happen. In
> case of mips it has already happened.
> 
> You can easily strip "arch_" part in the check_macroassembler_style.py
> 
> PS: If we want to merge mips32 and mips64 ports, as discussed in bug
> 1140954, we shouldn't rename current folder to mips32.

Vote++
(Assignee)

Comment 7

3 years ago
Created attachment 8634673 [details] [diff] [review]
bug1182936.patch
Attachment #8632699 - Attachment is obsolete: true
Attachment #8634673 - Flags: review?(nicolas.b.pierron)
(In reply to Branislav Rankov [:rankov] from comment #5)
> # define ALL_ARCH arch_mips, arch_arm, arch_arm64, arch_x86, arch_x64

This sounds like a noisy solution, as we would be repeating the "arch_" prefix many times in the file.

> You can easily strip "arch_" part in the check_macroassembler_style.py

True,

But we could have a "mips32" name and teach the script that mips32 macro name corresponds to the "mips" file name.  Which would be less invasive and less noisy for other architectures.
Comment on attachment 8634673 [details] [diff] [review]
bug1182936.patch

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

I really don't like these "arch_" prefixes in the MacroAssembler.h file.
I think making a particular case in the python script is fine, to avoid the collision with the MIPS macro.
Attachment #8634673 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 10

2 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> Comment on attachment 8634673 [details] [diff] [review]
> bug1182936.patch
> 
> Review of attachment 8634673 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I really don't like these "arch_" prefixes in the MacroAssembler.h file.
> I think making a particular case in the python script is fine, to avoid the
> collision with the MIPS macro.

OK, I have make a patch set to rename current mips to mips32. Next, split the code that can shared bweteen mips32 and mips64 to mips-shared directory. Last, push mips64 patches. 

It looks like x86 & x64, not only to fix this problem, but also make the code sharable.
(Assignee)

Comment 11

2 years ago
Created attachment 8646791 [details] [diff] [review]
0001-IonMonkey-MIPS32-Rename-mips-to-mips32.patch
Attachment #8634673 - Attachment is obsolete: true
Attachment #8646791 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 12

2 years ago
Created attachment 8646792 [details] [diff] [review]
0002-IonMonkey-MIPS32-Move-mips-macros-to-mips32.patch
Attachment #8646792 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 13

2 years ago
Created attachment 8646793 [details] [diff] [review]
0003-Rename-mips-to-mips32-in-check_macroassembler_style..patch

Thank you.
Attachment #8646793 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8646791 [details] [diff] [review]
0001-IonMonkey-MIPS32-Rename-mips-to-mips32.patch

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

Reviewing the patch with `git show -M90%` highlights no undesired changes, only header location changes.

PS: I think this is highest lines per hours ratio I made for a review :)
Attachment #8646791 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8646792 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8646793 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 15

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ed3076c89a
https://hg.mozilla.org/integration/mozilla-inbound/rev/159ca560b53c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccd5c8571773
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d9ed3076c89a
https://hg.mozilla.org/mozilla-central/rev/159ca560b53c
https://hg.mozilla.org/mozilla-central/rev/ccd5c8571773
Assignee: nobody → r
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.