oh nooooo… I have no idea what's happening, all the newlines in the previous
email were destroyed, sorry ugggh… let's try without disabling rich text in the
Samsung client this time :/Hi Martin, thanks for a quick and detailed
review!Oops, Thunderbird (or something else in the chain) has added an
extraneous space in the beginning of every line, though the +- deltas seem to
be unaffected. I can try sending the patch some other way if needed, like as a
binary attachment, or via git send-email, but possibly the "^ " (without
quotes) per-line regular expression should be fine for cleaning it up as well.
This is my first patch submission to this repository, and overall to a mailing
list, so I'm just starting learning the best practices… hi everyone!!!Yes, the
original issue was likely a consequence of us using a completely different
build process than FFmpeg is designed to be built with. We have our own Premake
5 scripts for FFmpeg components, that are used for generation of Android
ndk-build files via my Premake generator, Triang3l/premake-androidndk on
GitHub. So yes, we don't have any link options imported from a file.I agree, it
was very suprising to see that dynamic relocations were done between object
files within a single shared library at all. I suspected ASLR at first, but I
don't know if it can randomize the locations of individual object files this
way, that's probably not the case though. I'll see what can be done to
reproduce the effect of libavcodec.v — since my shared library is the final
application logic, not a library that's actually shareable, none of the FFmpeg
objects need to be exported from it at all. However, static libraries barely
have anything to configure overall as far as I know, so disabling exports
specifically for FFmpeg may be complicated — but thankfully, we can (and even
should, to reduce the file size) use -fvisibility=hidden globally in our
application, if that helps fix the issue. -Wl,-Bsymbolic should be fine too,
and possibly even less intrusive.If using __attribute__((visibility("hidden")))
for the lookup tables prevents dynamic relocations from being inserted, and if
that doesn't break common usages of libavcodec, yes, it should be a way better
solution than introducing an additional memory load at runtime. I'll check if
that works for us tomorrow or in a couple of days.If we're able to avoid using
the global object table this way though, maybe it could be desirable to also
get rid of `movrelx` in the AArch32 code as well?By the way, I'm also super
confused by how the offset is applied in the local `movrel` currently, it looks
very inconsistent. The `adrp` and `add` combination, as I understand its
operation, should work for any 32-bit literal value, not specifically for
addresses of known objects — `adrp` accepting the upper 20 bits as a literal
and adding them to the PC, and then `add` just adding the lower 12 bits, the
offset within the page, also taken as a literal.While `movrelx` most likely
requires the offset to be applied explicitly using 0…4095 `add` or `sub`
afterwards because it's first loaded from a lookup table of addresses of, if I
understand correctly, actual objects in the code (I wouldn't expect it to be
filled with object+8, object+16, object+24 pointers in case of a large object
for loading time reasons, though I haven't researched this topic in detail), if
everything `movrel` does is adding the PC to the input literal… do we even need
to emit `sub` for negative offsets in it?The current Linux implementation of
`movrel` just adds the offset directly to the label using + regardless of
whether it's positive or negative, and there already are instances of negative
offsets in the code (`subpel_filters` minus 16 in vp8dsp_neon.S). The AArch32
version of this code also doesn't use an offset parameter, rather, passing
`subpel_filters-16` directly as the label argument.However, the Apple AArch64
implementation does have two paths for applying the offset — directly to the
literal if it's positive, but via a `sub` instruction for a negative one. This
is also true for the Windows implementation — whose existence overall is
questionable, as Windows DLLs use a different relocation method, and PIC
doesn't apply to them at all if I understand correctly; and also the `movrel`
implementation for Windows with a non-negative offset is identical to the Linux
version, minus the hardware-assisted ASan path on Linux.I'll likely play with
the assembler later to see how negative offsets are interpreted, but still, is
there a reason to emit the subtraction instruction that you can remember, or
would it be safe to possibly even remove the offset argument completely?For
`movrelx`, however, if it's needed at all, the offset argument is desirable as
without PIC, it will be applied to the label literal for free.Thank you!— TriOn
Mon, 11 Jul 2022, Martin Storsjö wrote:> Hi,>> Thanks for your patch! While the
patch probably is worthwhile to pursue, > ffmpeg does work on Android 6 and
newer (with the aarch64 assembly), so > there's some gaps/misses in the
reasoning in the patch description.>> On Sun, 10 Jul 2022, Triang3l wrote:>>>
To support PIC, all AArch64 assembly code in FFmpeg uses the `movrel` macro >>
to obtain addresses of labels, such as lookup table constants, in a way >> that
with CONFIG_PIC being 1, PC-relative addresses of labels are computed >> via
the `adrp` instruction.>>> This approach, however, is suitable only for labels
defined in the same >> object file. For `adrp` to work directly between object
files, the linker >> has to perform a text relocation.>> No, it doesn't. It's
fully possible to build such libraries without text > relocations currently.>>
My memory of the situation was that we're linking our shared libraries with >
-Wl,-Bsymbolic, which means that those symbol references are bound at link >
time, so the offset from adrp to the target symbol is fixed at link time, and >
no text relocation is needed.>> However I did try to link such shared
libraries, removing the -Wl,-Bsymbolic > argument, and it still succeeds. So
I'm a bit unsure what really makes it > work for me when it isn't working for
you.Andreas Rheinhardt kindly reminded me that when linking libavcodec.so, we
add -Wl,--version-script,libavcodec/libavcodec.ver, which makes e.g. ff_cos_32
a non-exported symbol, which means that it can't be interposed, and thus the
relocation can be fixed at link time.If I remove that argument, I can reproduce
the linker errors, and adding -Wl,-Bsymbolic fixes it.I wonder if we could mark
these symbols as ELF hidden, so that they wouldn't need to be interposable at
all, even when you link the static library into a separate shared library?//
Martin
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
