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

[BUG] NDK r19 does not set armv7 arch with ndk-build #906

Closed
emmanuel-marty opened this issue Feb 8, 2019 · 8 comments
Closed

[BUG] NDK r19 does not set armv7 arch with ndk-build #906

emmanuel-marty opened this issue Feb 8, 2019 · 8 comments
Assignees
Milestone

Comments

@emmanuel-marty
Copy link


Description

This is minor, but when using ndk-build to compile native code with NDK r19 (probably due to switching to the stock toolchain) the assembler emits tons of warnings such as:

pngwrite-4abf69.s:1985: Rd and Rm should be different in mul
pngwrite-4abf69.s:7926: Rd and Rm should be different in mla
...

I understand these contraints date back from the armv5 architecture, and are irrelevant on armv7, so it seems that -march isn't correctly passed to clang, as the warnings go away if I add "-march-armv7-a" to LOCAL_CFLAGS in Android.mk.

I did not check if the fpu ABI flag is also incorrectly omitted due to the same issue, but that may be worth looking into as well.

All my new projects use Android Studio and cmake to build native code, and it's a warning that can be ignored, so that isn't a big deal, but I still have some legacy projects using ndk-build, maybe it's worth fixing at the NDK level so as not to pollute logs or have to add a conditional to add the -march flag to Android.mk

To reproduce:

  • Build C or C++ sources for the armv7-eabi target using ndk-build. The pnglib 1.6.36 sources trigger the issue in multiple locations for instance.

Environment Details

Not all of these will be relevant to every bug, but please provide as much
information as you can.

  • NDK Version: 19.0.5232133
  • Build system: ndk-build
  • Host OS: Mac (macOS 10.14.3)
  • Compiler: clang
  • ABI: armeabi-v7a
  • STL: c++_static
  • NDK API level: 16
  • Device API level: 28
@DanAlbert
Copy link
Member

That's strange. In a trivial ndk-build project I see no difference in compiler invocation based on the presence of -march armv7-a:

$ ndk-build APP_ABI=armeabi-v7a APP_CFLAGS=-v -B > without 2>&1
$ ndk-build APP_ABI=armeabi-v7a APP_CFLAGS="-march=armv7-a -v" -B > with 2>&1
$ diff with without
$

Presumably it's a problem for something other than Clang, but we don't ever invoke as directly so I'm not sure where it's coming from. I'll need to take a look at pnglib to see what's happening, but it looks like the feeling of panic I experienced when I thought we'd accidentally forced everyone back to ARM5 was premature :)

@DanAlbert DanAlbert self-assigned this Feb 8, 2019
@DanAlbert
Copy link
Member

@emmanuel-marty can you share the Android.mk/Application.mk you used for building libpng?

@emmanuel-marty
Copy link
Author

Hey there,

Apologies for the delay in answering. Here is a complete, reduced NDK project that exhibits the issue (I cut it down to just building libpng)

ndk_testcase.zip

I am also including the build log.

ndk_testcase_out.txt

As mentioned this is on macOS using ndk-build from NDK r19. This behavior didn't show on NDK r18b with the same config. It's probably a super minor issue (and doesn't show with my new projects using CMake and Android Studio as far as I can tell)

Thanks!

@emmanuel-marty
Copy link
Author

.. And here is a further cleaned up project actually, use this instead :-) I'm thinking maybe the use of -fno-integrated-as here may be a reason why you didn't see the behavior, in any case you have the .mk files in there and hopefully it will build fine for you and exhibit the issue.

ndk_testcase.zip

@DanAlbert
Copy link
Member

-fno-integrated-as is indeed the problem. Clang doesn't forward the implicit architecture flag on to the assembler.

I've already sent r19b to QA, so it may be too late to get it fixed for that release, but I'll check on that and at the very least get this added to the KI list.

Note that the non-integrated assembler is not the default, so this shouldn't affect most builds.

@DanAlbert DanAlbert added this to the r19b milestone Feb 12, 2019
@DanAlbert
Copy link
Member

DanAlbert commented Feb 12, 2019

Looks like I caught it in time. I'll fix this for r19b, which will unfortunately delay r19b a day or so, but is worth doing IMO.

@emmanuel-marty
Copy link
Author

It was definitely a rare combination of the now less used ndk-build tool and non-default flags (that particular project used -fno-integrated-as to properly assemble arm .S files for libtheora, which I cut out of the test project for clarity) but still great that you were able to fix it in time for r19b. Thanks!

@DanAlbert
Copy link
Member

Fix is merged to r19. Thanks for the report!

disigma pushed a commit to wimal-build/ndk that referenced this issue Feb 22, 2019
The non-integrated assembler does not get the proper -march flag when
it is implicit in the triple. Pass it explicitly to avoid problems
until we can fix the driver.

Writing a test that fails to compile is proving difficult because the
warnings emitted by the assembler for the provided test case are just
notes rather than warnings so they can't be promoted to errors, and we
can't use the __ARM_ARCH_7A__ define because those are resolved by the
C preprocessor, which *does* have the right architecture set...

Will attempt to add tests for this in a follow up since we need to
respin r19b ASAP.

Test: Manual inspection for now.
Bug: android/ndk#906
Change-Id: I4ab3e9aee0775150731108e42f5f584a5f3c27ea
(cherry picked from commit e3f7078)
mehulagg pushed a commit to mehulagg/superproject that referenced this issue Dec 21, 2019
* Update ndk from branch 'ndk-release-r19'
  to 01e24d4dd3f72c3743ea964de6a965af7909e816
  - Merge "Fix ARM version for non-integrated assembler." into ndk-release-r19
  - Fix ARM version for non-integrated assembler.
    
    The non-integrated assembler does not get the proper -march flag when
    it is implicit in the triple. Pass it explicitly to avoid problems
    until we can fix the driver.
    
    Writing a test that fails to compile is proving difficult because the
    warnings emitted by the assembler for the provided test case are just
    notes rather than warnings so they can't be promoted to errors, and we
    can't use the __ARM_ARCH_7A__ define because those are resolved by the
    C preprocessor, which *does* have the right architecture set...
    
    Will attempt to add tests for this in a follow up since we need to
    respin r19b ASAP.
    
    Test: Manual inspection for now.
    Bug: android/ndk#906
    Change-Id: I4ab3e9aee0775150731108e42f5f584a5f3c27ea
    (cherry picked from commit e3f70782abf6e4567aa1cfc025b001d3c125f06f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants