The default bug view has changed. See this FAQ.

Fixes for nanojit on ARMv4T

RESOLVED FIXED in mozilla9

Status

Core Graveyard
Nanojit
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla9
ARM
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-nanojit][inbound][fixed-in-tamarin])

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
In bug 547063, NJ_COMPILER_ARM_ARCH was reinstated, but it checked that the arm architecture was >= 5, leaving out ARMv4T support, which still exists.
(Assignee)

Comment 1

6 years ago
(In reply to comment #0)
> In bug 547063, NJ_COMPILER_ARM_ARCH was reinstated

+static assertion
(Assignee)

Comment 2

6 years ago
Created attachment 544965 [details] [diff] [review]
Fixes for nanojit on ARMv4T

(Not convinced you are the right reviewer for this)

This fixes NJ_COMPILER_ARM_ARCH static assertion. At the same time, also push some binary target correctness: each object file is marked with the target it was built for, and when linking, ld marks the final binary with the greater target it found on the way. With the hacks to allow building unsupported instructions on the target used by the compiler, the resulting objects end up having a bumped target, thus bumping the final binary (libxul) target, which is not expected. Using .object_arch forces the target to be downsized for the given object file, but doesn't affect the final binary target, since other object files are still targetted at whatever target is used by the compiler.
Attachment #544965 - Flags: review?(doug.turner)

Updated

6 years ago
Attachment #544965 - Flags: review?(doug.turner) → review?(cdleary)
Comment on attachment 544965 [details] [diff] [review]
Fixes for nanojit on ARMv4T

Passing the buck to Nick, because I know nothing about NanoJIT or how things work upstream. jbramley has also done work on NanoJIT for ARM, so he can feel free to steal.
Attachment #544965 - Flags: review?(nnethercote)
Attachment #544965 - Flags: review?(cdleary)
Attachment #544965 - Flags: feedback?(Jacob.Bramley)
Comment on attachment 544965 [details] [diff] [review]
Fixes for nanojit on ARMv4T

My understanding was that Nanojit isn't intended to support anything less than ARMv5.  Why do you want this?  More targets means more testing.  The trend we've been following is away from supporting older ARM versions.

And I don't know what the .object_arch thing does, so asking Jacob for proper review.  Also asking Ed Smith for feedback on Adobe's view.
Attachment #544965 - Flags: review?(nnethercote)
Attachment #544965 - Flags: review?(Jacob.Bramley)
Attachment #544965 - Flags: feedback?(edwsmith)
Attachment #544965 - Flags: feedback?(Jacob.Bramley)
Attachment #544965 - Flags: feedback-
(Assignee)

Comment 5

6 years ago
Actually, ARMv4T support was added a while ago in bug 552624, and AFAIK, still works. The testing is done on Debian build bots.
Comment on attachment 544965 [details] [diff] [review]
Fixes for nanojit on ARMv4T

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

::: js/src/nanojit/NativeARM.cpp
@@ +130,5 @@
>          // target smaller than armv7, despite it being legal for armv5+.
>              "   .arch armv7-a\n"
> +        // Force the object to be tagged as armv4t to avoid it bumping the final
> +        // binary target.
> +            "   .object_arch armv4t\n"

What happens if the rest of the file is built for ARMv5T? Won't the final object end up being marked as ARMv4T with unconditional ARMv5 code being generated from the C functions? Pixman uses the same technique so presumably it has the same issues. Do you know how this is this handled elsewhere?

It'd be much cleaner if we could have a method of emitting the ARMv5 instruction without affecting any of the object meta-data, but that doesn't seem to exist. (The only work-around I know of is to use .word to hard-encode the instruction, but then you lose register allocation.)

It looks like Siarhei has done this before (in Pixman) so I'd like to defer to him for more insight.
Attachment #544965 - Flags: review?(Jacob.Bramley) → review?(siarhei.siamashka)
(In reply to comment #4)
> Comment on attachment 544965 [details] [diff] [review] [review]
> Fixes for nanojit on ARMv4T
> 
> My understanding was that Nanojit isn't intended to support anything less
> than ARMv5.  Why do you want this?  More targets means more testing.  The
> trend we've been following is away from supporting older ARM versions.

We removed ARMv4T about two-three years ago because we thought no-one used it, and it was a nuisance to maintain. It turned out that Debian still want it, so Mike reinstated ARMv4T support a while back. Strangely, I can't find either bug now.
(Assignee)

Comment 8

6 years ago
Yes, the whole object file will be marked as armv4t with armv5t code in it, but as long as as there is at least one other source file not containing .object_arch, the linker will mark the final binary as armv5t.
(In reply to comment #8)
> Yes, the whole object file will be marked as armv4t with armv5t code in it,
> but as long as as there is at least one other source file not containing
> .object_arch, the linker will mark the final binary as armv5t.

Ok. It still doesn't feel right somehow, but I can't think of a better way around it and we will always link with some other object, so I'll give this r+.

(In reply to comment #7)
> Strangely, I can't find either bug now.

Oh, Mike already linked the bug where support was reinstated.
Attachment #544965 - Flags: review?(siarhei.siamashka)
Attachment #544965 - Flags: review+
Attachment #544965 - Flags: feedback?(siarhei.siamashka)

Comment 10

6 years ago
(In reply to comment #6)
> What happens if the rest of the file is built for ARMv5T? Won't the final
> object end up being marked as ARMv4T with unconditional ARMv5 code being
> generated from the C functions? Pixman uses the same technique so presumably
> it has the same issues. Do you know how this is this handled elsewhere?

"ELF for the ARM Architecture" - http://infocenter.arm.com/help/topic/com.arm.doc.ihi0044d/index.html

"Addenda to, and Errata in, the ABI for the ARM Architecture" - http://infocenter.arm.com/help/topic/com.arm.doc.ihi0045c/index.html

The former describes how Call and Jump relocations may use BLX instruction (and it is not supported by armv4t).

The latter describes the ELF build attributes and how they are combined.

> It'd be much cleaner if we could have a method of emitting the ARMv5
> instruction without affecting any of the object meta-data, but that doesn't
> seem to exist. (The only work-around I know of is to use .word to
> hard-encode the instruction, but then you lose register allocation.)
> 
> It looks like Siarhei has done this before (in Pixman) so I'd like to defer
> to him for more insight.

The problem with pixman was that linking any object files compiled for anything newer than armv4t was causing SIGILL problems for armv4t due to BLX instructions getting generated when resolving relocations.

According to "info as":

`.object_arch NAME'
     Override the architecture recorded in the EABI object attribute
     section.  Valid values for NAME are the same as for the `.arch'
     directive.  Typically this is useful when code uses runtime
     detection of CPU features.

Because "runtime detection of CPU features" is used in both pixman and nanojit, this looks like an acceptable solution. Emitting these directives in inline assembly code is indeed a bit intrusive and not very clean. For example, I wonder if this is going to be compatible with the native non-gnu assembler from llvm/clang? But for now I don't see anything better.
(Assignee)

Comment 11

6 years ago
(In reply to comment #10)
> The problem with pixman was that linking any object files compiled for
> anything newer than armv4t was causing SIGILL problems for armv4t due to BLX
> instructions getting generated when resolving relocations.

If the compiler is targeting armv4t, BLX is not going to be emitted even if you have .arch armv7-a as asm in your C(++) code. The code emitted by the compiler will still be armv4t. But with .arm armv7-a, the assembler records armv7-a as the architecture in the EABI object attribute section for the corresponding object, and the linker then records that in the final binary, while the code is actually armv4t, with a few bits of armv7-a, that is only run if the runtime detection finds armv7-a/neon/whatever support.

If the compiler is targeting something higher than armv4t, let's say armv5t, it will emit BLX instructions. .object_arch armv4t will then make the resulting object marked as an armv4t object, which it actually isn't, since it contains BLX instructions. However, as the other object files would be armv5t, the linker would mark the final binary as armv5t.

> According to "info as":
> 
> `.object_arch NAME'
>      Override the architecture recorded in the EABI object attribute
>      section.  Valid values for NAME are the same as for the `.arch'
>      directive.  Typically this is useful when code uses runtime
>      detection of CPU features.
> 
> Because "runtime detection of CPU features" is used in both pixman and
> nanojit, this looks like an acceptable solution. Emitting these directives
> in inline assembly code is indeed a bit intrusive and not very clean. For
> example, I wonder if this is going to be compatible with the native non-gnu
> assembler from llvm/clang? But for now I don't see anything better.

Considering the mozilla code already builds fine with clang, I guess it is compatible (since our copy of pixman has that)
(Assignee)

Comment 12

6 years ago
Now, the question is, where should I land this? is m-i fine? or should that go to nanojit? or both?

Comment 13

6 years ago
(In reply to comment #11)
> (In reply to comment #10)
> > The problem with pixman was that linking any object files compiled for
> > anything newer than armv4t was causing SIGILL problems for armv4t due to BLX
> > instructions getting generated when resolving relocations.
> 
> If the compiler is targeting armv4t, BLX is not going to be emitted even if
> you have .arch armv7-a as asm in your C(++) code. The code emitted by the
> compiler will still be armv4t. But with .arm armv7-a, the assembler records
> armv7-a as the architecture in the EABI object attribute section for the
> corresponding object, and the linker then records that in the final binary,
> while the code is actually armv4t, with a few bits of armv7-a, that is only
> run if the runtime detection finds armv7-a/neon/whatever support.
> 
> If the compiler is targeting something higher than armv4t, let's say armv5t,
> it will emit BLX instructions. .object_arch armv4t will then make the
> resulting object marked as an armv4t object, which it actually isn't, since
> it contains BLX instructions. However, as the other object files would be
> armv5t, the linker would mark the final binary as armv5t.

BLX is generated not by the compiler, but by the linker. Here was a testcase:
    http://www.mail-archive.com/pixman@lists.freedesktop.org/msg00094.html

And this linker behavior is triggered for the resulting binary by having just any single object file having >=armv5t elf attribute linked with the rest. It's a real problem because this issue affects whe whole code, not just the parts selected by runtime cpu features detection. And this bug was encountered in the wild and reported against pixman some time ago.
(Assignee)

Comment 14

6 years ago
Well that one is kind of special because it's mixing arm and thumb. The case here isn't.

Comment 15

6 years ago
(In reply to comment #14)
> Well that one is kind of special because it's mixing arm and thumb. The case
> here isn't.

Yes, if building Mozilla for armv4t thumb1 is unsupported, then it's likely not an issue. But for pixman it was, because the libraries are better to work correctly in any possible environment no matter how the end user (distro maintainer) decides to build them.
(Assignee)

Comment 16

6 years ago
http://hg.mozilla.org/projects/nanojit-central/rev/6943e40c4f0a
Assignee: nobody → mh+mozilla
Whiteboard: fixed-in-nanojit

Comment 17

6 years ago
changeset: 6521:f0b138ce823a
user:      Mike Hommey <mh+mozilla@glandium.org>
summary:   Bug 670323 - Fixes for nanojit on ARMv4T. r=jbramley

http://hg.mozilla.org/tamarin-redux/rev/f0b138ce823a
http://hg.mozilla.org/mozilla-central/rev/cd22ad961887
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
http://hg.mozilla.org/integration/mozilla-inbound/rev/cd22ad961887
Whiteboard: fixed-in-nanojit → [fixed-in-nanojit][inbound][fixed-in-tamarin]

Comment 20

6 years ago
Comment on attachment 544965 [details] [diff] [review]
Fixes for nanojit on ARMv4T

better late than never... this all seems fine.
Attachment #544965 - Flags: feedback?(edwsmith) → feedback+
Attachment #544965 - Flags: feedback?(siarhei.siamashka)
Component: Nanojit → Nanojit
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.