Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch Android build to using clang (needs buildbot update) #8651

Merged
merged 7 commits into from
Jul 24, 2016

Conversation

unknownbrackets
Copy link
Collaborator

I'm least certain about the VAO func changes (aapcs-vfp), but I think it should be safe.

Did not fix all the clang warnings, just some as I was trying to get it to compile. Have not really tested on a device yet.

-[Unknown]

@unknownbrackets unknownbrackets changed the title Switch Android build to using clang BROKEN - Switch Android build to using clang Mar 20, 2016
@unknownbrackets
Copy link
Collaborator Author

Hmm, it seems not to work on my device properly... it fails to link properly.

Also travis and buildbot would need updating to r11b, otherwise they may fail linking on some __thread code, although apparently TLS doesn't work in clang right now, actually.

-[Unknown]

@mrcmunir
Copy link
Contributor

For me Crash too me too when linked ffmpeg

/opt/android-ndk/toolchains/x86_64-4.9/prebuilt/linux-x86_64/lib/gcc/x86_64-linux-android/4.9/../../../../x86_64-linux-android/bin/ld: error: jni/../../ffmpeg/android/x86_64/lib/libavcodec.a(deinterlace.o): requires dynamic R_X86_64_PC32 reloc against 'ff_pw_4' which may overflow at runtime

Later i Try with -fPIC if resolved this problem.

With -fPIC Fixed this warning error later we need compile ffmpeg with clang .

@sum2012
Copy link
Collaborator

sum2012 commented Mar 21, 2016

I compile error with r11b ndk
2

@mrcmunir
Copy link
Contributor

Yes it's broken because we needs recompile switch ffmpeg too with clang .

I'm investigate this how compile with clang ffmpeg library.

@mrcmunir
Copy link
Contributor

I'm miss for arm64 they don't like asm code with clang

./libavutil/aarch64/bswap.h:31:13: error: invalid instruction mnemonic 'rev16'
asm("rev16 %w0, %w0" : "+r"(x));
^
:1:2: note: instantiated into assembly here
rev16 %di, %di
^~~~~
In file included from libavformat/aacdec.c:23:
In file included from ./libavutil/intreadwrite.h:25:
In file included from ./libavutil/bswap.h:38:
./libavutil/aarch64/bswap.h:38:13: error: invalid instruction mnemonic 'rev'
asm("rev %w0, %w0" : "+r"(x));

How fix it?

This problem is related with Arm64 only others architecture compile ffmpeg ok with clang in my internal test

@unknownbrackets unknownbrackets force-pushed the android-clang branch 4 times, most recently from d55fd46 to dbd1d80 Compare April 24, 2016 17:39
@unknownbrackets
Copy link
Collaborator Author

This works fine now (with r11c), but there's still the ffmpeg problem. Ideally, ffmpeg needs to be rebuilt with android-9 and with --disable-asm in x86.

-[Unknown]

@hrydgard
Copy link
Owner

ok, I'll take care of rebuilding ffmpeg soon.

@unknownbrackets
Copy link
Collaborator Author

See #8677 (comment) for notes about switching FFmpeg to clang. Looks doable.

With that we'd only need the NDK updated on the buildbot (#8677 must be fixed first.)

One odd thing, in my tests I'm using everything the same - but I don't get the pic errors on x86_64, even without rebuilding ffmpeg for x86_64... unlike https://travis-ci.org/hrydgard/ppsspp/jobs/131943336. But, it builds also with clang.

Apparently FFmpeg is at 3.0 now and includes "asm optimizations": http://ffmpeg.org/#pr3.0

-[Unknown]

@unknownbrackets unknownbrackets changed the title BROKEN - Switch Android build to using clang Switch Android build to using clang (needs buildtbot update) May 23, 2016
@unknownbrackets unknownbrackets changed the title Switch Android build to using clang (needs buildtbot update) Switch Android build to using clang (needs buildbot update) May 23, 2016
@unknownbrackets
Copy link
Collaborator Author

unknownbrackets commented May 23, 2016

I tried 3.0, and it's probably worth updating to given we'd need to recompile a bunch of things anyway, but it doesn't help the x86 issue.

Anyway, on my local, all Android platforms link fine, but on Travis (even with the same binaries), they don't. I'm not sure why. I've tried cleaning my objs and also cleaning the ccache in Travis without improvement. I definitely did recompile it with --enable-pic.

Not sure what I might've done wrong. Maybe should just wait for r12...

-[Unknown]

@mrcmunir
Copy link
Contributor

mrcmunir commented May 23, 2016

I Have too with linked problem with Clang compiler
NOTE : Android-x64 ffmpeg script missing --enable-pic Maybe we needs recompile x86_64 with clang compiler.

Edit : strange I tried with -fpic or --enable-pic and gives me the same error

@mrcmunir
Copy link
Contributor

mrcmunir commented May 23, 2016

Well for some part good news disable asm ffmpeg code in x86-32bits load fine under marshamllow-x86

I tested in my notebook with latest android-x86 changes Kernel 4.4.10 + mesa driver 11.2.2 + android 6.0.1 r43 version patched security may 1.

tmp_3969-screenshot_20160523-132224-183916702

tmp_3969-screenshot_20160523-132922-150526160

tmp_3969-screenshot_20160523-1332091405673383

FF7 crisis core test 5x resolution and HW 2x scaler full speed without problem in x86 lib.

tmp_3969-screenshot_20160523-125844-1669866591

For other bad part x86_64 won't linked fine with clang for some reason this problem no exist with GCC 4.9.

@unknownbrackets
Copy link
Collaborator Author

unknownbrackets commented May 28, 2016

@maximu Dumb mistake, I forgot I had tried ec491ef as an alternative to --disable-asm and never removed it. That was what made x86_64 build correctly (although it doesn't allow asm on x86.)

It seems FFmpeg wants to be linked with -Wl,-Bsymbolic, not sure if that has any dangerous side effects for us. That also makes it link correctly on x86_64.

-[Unknown]

@unknownbrackets unknownbrackets force-pushed the android-clang branch 2 times, most recently from 62e8e91 to e130757 Compare May 28, 2016 20:00
@mrcmunir
Copy link
Contributor

mrcmunir commented May 28, 2016

@unknownbrackets Ah I see good find with this ec491ef change compile fine with clang compiler 👍

Edit : After Tested hours later no detected any problem in android version .

For Linux version not sure if my distro problem I tested under archlinux no sound here I need rebuild ffmpeg files for sound working fine.

Also I detected little speed up ffmpeg performance especially noticeable in the loads that the buffer is much cleaner.

@unknownbrackets
Copy link
Collaborator Author

unknownbrackets commented Jun 7, 2016

@hrydgard only things remaining for this are:

  • Buildbot update to latest NDK (in perfect world, might wait for r12)
  • Ideally update FFmpeg on all platforms (this updates most, but not Windows/Mac/iOS)
  • Linux sound issue (or could rollback to old FFmpeg libs there)

-[Unknown]

@mrcmunir
Copy link
Contributor

NDK r12 released out.
Clang has been updated to 3.8
Now they use by default : -Wl,--warn-shared-textrel and -Wl,--fatal-warnings for 32bit architectures

@hrydgard
Copy link
Owner

Cool, guess it's time .. I won't have time to do the work until next week unfortunately.

@Orphis , feel like getting the buildbot ready for NDK 12 in the meantime? Watch out - Google has managed to swap their windows 32-bit and 64-bit downloads on their NDK download page...

@unknownbrackets
Copy link
Collaborator Author

unknownbrackets commented Jul 24, 2016

I'll rebase this after hrydgard/ppsspp-ffmpeg#39 is merged, and pull out the Travis/Linux experiment (b6ca610.) I'll also update it to use r12b.

I think we might have to keep the unused exports thing, at least for x64. I can experiment with making that x64 only.

I think we still need the buildbot update...

-[Unknown]

It's now the recommended build from NDK 11+.  GCC is deprecated.
This resolves a linkage error with x64 affecting Marshmallow and clang.
It also makes the unexported symbols non-visible since they don't need to
be - this reduces the size of the ppsspp_jni.so too.

Unfortunately, it reduces the readability of stack traces.
@hrydgard hrydgard added this to the v1.3.0 milestone Jul 24, 2016
@hrydgard hrydgard merged commit a1e815e into hrydgard:master Jul 24, 2016
@hrydgard
Copy link
Owner

Thanks for updating the branch. Hopefully we will run fine on Nexus Player now :)

@unknownbrackets unknownbrackets deleted the android-clang branch July 24, 2016 20:15
@unknownbrackets
Copy link
Collaborator Author

@Orphis not seeing r12 or r12b on the buildbot - no env var, and trying to build with the dir fails.

-[Unknown]

@unknownbrackets
Copy link
Collaborator Author

Just an update: buildbot is all working now.

-[Unknown]

@unknownbrackets
Copy link
Collaborator Author

Someone reported worse performance in FIFA 14 (and unnamed other games) after this commit (not sure if Clang or FFmpeg):

http://forums.ppsspp.org/showthread.php?tid=20315

We could try switching back from clang - although I haven't yet reproduced the performance difference.

-[Unknown]

@hrydgard
Copy link
Owner

hrydgard commented Aug 21, 2016

We're not gonna switch back. GCC is deprecated for Android now so we'll just have to live with it. If we can figure out a specific part that got slower, we might be able to optimize it back up. Or there might be some compiler option to change...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants