Last Comment Bug 670323 - Fixes for nanojit on ARMv4T
: Fixes for nanojit on ARMv4T
Status: RESOLVED FIXED
[fixed-in-nanojit][inbound][fixed-in-...
:
Product: Core Graveyard
Classification: Graveyard
Component: Nanojit (show other bugs)
: Trunk
: ARM Linux
: -- normal (vote)
: mozilla9
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on: 552624
Blocks: 547063
  Show dependency treegraph
 
Reported: 2011-07-09 00:55 PDT by Mike Hommey [:glandium]
Modified: 2014-03-17 08:00 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fixes for nanojit on ARMv4T (2.30 KB, patch)
2011-07-09 01:02 PDT, Mike Hommey [:glandium]
Jacob.Bramley: review+
n.nethercote: feedback-
edwsmith: feedback+
Details | Diff | Review

Description Mike Hommey [:glandium] 2011-07-09 00:55:25 PDT
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.
Comment 1 Mike Hommey [:glandium] 2011-07-09 00:58:28 PDT
(In reply to comment #0)
> In bug 547063, NJ_COMPILER_ARM_ARCH was reinstated

+static assertion
Comment 2 Mike Hommey [:glandium] 2011-07-09 01:02:43 PDT
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.
Comment 3 Chris Leary [:cdleary] (not checking bugmail) 2011-07-12 14:22:12 PDT
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.
Comment 4 Nicholas Nethercote [:njn] 2011-07-12 17:40:26 PDT
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.
Comment 5 Mike Hommey [:glandium] 2011-07-12 23:08:03 PDT
Actually, ARMv4T support was added a while ago in bug 552624, and AFAIK, still works. The testing is done on Debian build bots.
Comment 6 Jacob Bramley [:jbramley] 2011-07-13 00:59:19 PDT
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.
Comment 7 Jacob Bramley [:jbramley] 2011-07-13 01:12:54 PDT
(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.
Comment 8 Mike Hommey [:glandium] 2011-07-13 01:14:16 PDT
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.
Comment 9 Jacob Bramley [:jbramley] 2011-07-13 01:19:39 PDT
(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.
Comment 10 Siarhei Siamashka 2011-07-15 04:30:03 PDT
(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.
Comment 11 Mike Hommey [:glandium] 2011-07-15 04:58:09 PDT
(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)
Comment 12 Mike Hommey [:glandium] 2011-07-15 04:59:20 PDT
Now, the question is, where should I land this? is m-i fine? or should that go to nanojit? or both?
Comment 13 Siarhei Siamashka 2011-07-15 05:33:09 PDT
(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.
Comment 14 Mike Hommey [:glandium] 2011-07-15 06:06:39 PDT
Well that one is kind of special because it's mixing arm and thumb. The case here isn't.
Comment 15 Siarhei Siamashka 2011-07-15 06:11:36 PDT
(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.
Comment 16 Mike Hommey [:glandium] 2011-07-22 00:43:48 PDT
http://hg.mozilla.org/projects/nanojit-central/rev/6943e40c4f0a
Comment 17 Tamarin Bot 2011-08-17 17:16:59 PDT
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
Comment 18 Marco Bonardo [::mak] 2011-08-24 02:03:02 PDT
http://hg.mozilla.org/mozilla-central/rev/cd22ad961887
Comment 19 Nicholas Nethercote [:njn] 2011-08-24 02:45:19 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/cd22ad961887
Comment 20 Edwin Smith 2011-08-24 07:51:15 PDT
Comment on attachment 544965 [details] [diff] [review]
Fixes for nanojit on ARMv4T

better late than never... this all seems fine.

Note You need to log in before you can comment on or make changes to this bug.