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 clang building release raise error declaration does not match while llvm upstream compiles #2013

Closed
yujincheng08 opened this issue Apr 12, 2024 · 8 comments
Labels

Comments

@yujincheng08
Copy link

yujincheng08 commented Apr 12, 2024

Description

PoC: https://github.com/yujincheng08/ndkPoC

When we run buildCMakeRelWithDebInfo it raises the error while running buildCmakeDebug it compiles.

However, the upstream does work: https://godbolt.org/z/MMfq84rcK

Error message:

Execution failed for task ':app:buildCMakeRelWithDebInfo[x86_64]'.
> com.android.ide.common.process.ProcessException: ninja: Entering directory `C:\Users\loves\AndroidStudioProjects\NativeTest\app\.cxx\RelWithDebInfo\1i6i364f\x86_64'
  [1/2] Building CXX object CMakeFiles/test.dir/foo.cc.o
  FAILED: CMakeFiles/test.dir/foo.cc.o 
  D:\Android\SDK\ndk\28.0.11699160\toolchains\llvm\prebuilt\windows-x86_64\bin\clang++.exe --target=x86_64-none-linux-android24 --sysroot=D:/Android/SDK/ndk/28.0.11699160/toolchains/llvm/prebuilt/windows-x86_64/sysroot -Dtest_EXPORTS  -g -DANDROID -fdata-sections -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security   -O2 -g -DNDEBUG -std=gnu++20 -fPIC -MD -MT CMakeFiles/test.dir/foo.cc.o -MF CMakeFiles\test.dir\foo.cc.o.d @CMakeFiles\test.dir\foo.cc.o.modmap -o CMakeFiles/test.dir/foo.cc.o -c C:/Users/loves/AndroidStudioProjects/NativeTest/app/src/main/cpp/foo.cc
  In module 'bar' imported from C:/Users/loves/AndroidStudioProjects/NativeTest/app/src/main/cpp/foo.cc:3:
  C:/Users/loves/AndroidStudioProjects/NativeTest/app/src/main/cpp/h.hpp:6:34: error: 'S::s' from module 'bar.<global>' is not present in definition of 'S<c...>' provided earlier
      6 |     inline static constexpr char s[]{c...};
        |                                  ^
  C:/Users/loves/AndroidStudioProjects/NativeTest/app/src/main/cpp/h.hpp:6:34: note: declaration of 's' does not match
      6 |     inline static constexpr char s[]{c...};
        |                                  ^
  1 error generated.
  ninja: build stopped: subcommand failed.
  
  C++ build system [build] failed while executing:
      @echo off
      "C:\\Program Files\\CMake\\bin\\ninja.exe" ^
        -C ^
        "C:\\Users\\loves\\AndroidStudioProjects\\NativeTest\\app\\.cxx\\RelWithDebInfo\\1i6i364f\\x86_64" ^
        test
    from C:\Users\loves\AndroidStudioProjects\NativeTest\app

Upstream bug

llvm/llvm-project#79240

Commit to cherry-pick

llvm/llvm-project#80249

Affected versions

Canary

Canary version

28.0.11706545

Host OS

Linux, Windows

Host OS version

11

Affected ABIs

armeabi-v7a, arm64-v8a, x86, x86_64

@yujincheng08 yujincheng08 reopened this Apr 12, 2024
@yujincheng08 yujincheng08 changed the title [Bug] ndk clang raise error declaration does not match while llvm upstream compiles [Bug] ndk clang building release raise error declaration does not match while llvm upstream compiles Apr 12, 2024
@appujee
Copy link
Collaborator

appujee commented Apr 12, 2024

It doesn't look like a regression from previous compiler: https://godbolt.org/z/P8j4GnhYE This has been fixed recently in upstream clang.

@yujincheng08
Copy link
Author

yujincheng08 commented Apr 12, 2024

@appujee yes, and i have bisected the commit that fixed this issue. so maybe we need to cherry-pick this fix.

@DanAlbert
Copy link
Member

I'll defer to the LLVM folks on whether or not that's innocuous enough to cherry-pick to r27. It's already in QA, so the bar for cherry-picks of non-regressions has gone up.

@pirama-arumuga-nainar
Copy link
Collaborator

a0e1cc0aefbc mentioned in the original report is the back port to the llvm-18 branch. a0b6747804e466 is the patch to the main branch of llvm-project.

@yujincheng08
Copy link
Author

So is it possible to pick it to r27?

@pirama-arumuga-nainar
Copy link
Collaborator

https://android-review.googlesource.com/c/toolchain/llvm_android/+/3042676 is the cherry-pick to AOSP clang. Let's see if it passes presubmit and tests.

The fix seems reasonable. However, since this is not a regression, it doesn't meet the bar for a compiler respin. If there is another bug in the compiler that requires updating the r27 clang, we'll include the fix for this as well.

@DanAlbert
Copy link
Member

(and there almost always is another bug, so it's likely)

bvlgah pushed a commit to bvlgah/llvm_android_mirror that referenced this issue Apr 19, 2024
a0b6747804e [C++20] [Modules] Don't perform ODR checks in GMF

Note: clang/test/Modules/pr76638.cppm skipped as it doesn't exist at
r522817.

This change is generated automatically by the script:
  cherrypick_cl.py --sha a0b6747804e46665ecfd00295b60432bfe1775b6 --bug android/ndk#2013 --patch-file ../../a0b6747804e46665ecfd00295b60432bfe1775b6_new.patch --create-cl

Bug: android/ndk#2013

Test: N/A
Change-Id: I24e49f94c4eba8e3d2f708fe2f0f7427f34ee084
@yujincheng08
Copy link
Author

Confirmed 28.0.11744028 works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Merged
Status: Triaged
Development

No branches or pull requests

4 participants