Disable execinfo.h include for Android build in WinAdapter.h

[seppala2]

This small change makes it possible to build and use DirectXShaderCompiler on Android, at least on arm64-v8a.

execinfo.h does not exist in Android toolchain. However it is only used by clang-hlsl-tests which can be disabled from the build.

I hope this could be merged, or fixed in another way, since being able to load the compiler on Android can help development quite a bit, even though for a shipped version the binary (~20 MB) would be maybe a bit too large to include in smaller apps (and glslang is not always an option as new HLSL features seem to land into DXC first). I've already used the Android build for debugging shader related issues quickly by hot-reloading shader source code changes directly on the Android device without having to restart the app and compile shaders manually on the host.

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.150 completed (commit 62c6c29536 by @)

[pow2clk]

Ownership of the Linux build is a bit mixed, but especially since this is Android, I'm going to loop @jaebaek in.

I have a mild preference for not making this conditional on Android, but rather on either the ability to print the stack or building tests as determined by the cmake configuration which percolates into #defines. That way the platform can be used in the cmake files to force disable that feature, which will exclude this header.

[jaebaek]

I have never tested DXC on Android. It sounds interesting idea.
If you already tested it, it looks OK to me, but it would be better to follow the guidance of @pow2clk .

[seppala2]

I actually looked into the usage of CaptureStackTrace more, and realized that the only place in the whole solution where it is used is CompilerTest.cpp in clang-hlsl-tests. This file is already disabled from non-windows builds, see this part in clang/unittests/HLSL/CMakeLists.txt:

if(WIN32)
set(HLSL_IGNORE_SOURCES
  TestMain.cpp
  HLSLTestOptions.cpp
)
add_clang_library(clang-hlsl-tests SHARED
  AllocatorTest.cpp
  CompilerTest.cpp
  DxilContainerTest.cpp
  DxilModuleTest.cpp
  DXIsenseTest.cpp
  ExecutionTest.cpp
  ExtensionTest.cpp
  FunctionTest.cpp
  LinkerTest.cpp
  MSFileSysTest.cpp
  Objects.cpp
  OptimizerTest.cpp
  OptionsTest.cpp
  PixTest.cpp
  RewriterTest.cpp
  ShaderOpTest.cpp
  SystemValueTest.cpp
  ValidationTest.cpp
  VerifierTest.cpp
  clang-hlsl-tests.rc
  )
else (WIN32)
set(HLSL_IGNORE_SOURCES
  ExecutionTest.cpp
  LinkerTest.cpp
  MSFileSysTest.cpp
  PixTest.cpp
  RewriterTest.cpp
  ShaderOpTest.cpp
  DxilContainerTest.cpp
  ValidationTest.cpp
  CompilerTest.cpp
  )

add_clang_unittest(clang-hlsl-tests
  AllocatorTest.cpp
  DxilModuleTest.cpp
  DXIsenseTest.cpp
  ExtensionTest.cpp
  FunctionTest.cpp
  HLSLTestOptions.cpp
  Objects.cpp
  OptimizerTest.cpp
  OptionsTest.cpp
  SystemValueTest.cpp
  TestMain.cpp
  VerifierTest.cpp
  )

endif(WIN32)

If you look at the non-win32 branch in the CMakeLists, CompilerTest.cpp is not even included. I can actually build the whole solution for Android, and since the CompilerTest.cpp is not included in non-Windows builds, I think the whole execinfo.h include and CaptureStackBackTrace macro could be removed from WinAdapter.h without breaking anything in Linux/Mac builds either.

@jaebaek Should the execinfo.h include be just removed completely from WinAdapter.h since it looks like it is not really needed for non-Windows builds ATM? Or considering that the Android build works, without having to disable any target in the solution, could it be fine to just add the #ifndef ANDROID as in this pull request?

@pow2clk Considering that the CaptureStackBackTrace is not needed at all in non-Windows builds, implementing this feature for Android seems unnecessary, and adding a custom define for it would be unnecessary since the condition is always true: execinfo is never needed in non-windows builds because the parts where it is used are disabled from the CMakeLists already.

I tested this on Galaxy S21 (Exynos), compiled for NDK 21.1.6352462, target SDK 21. I didn't run the tests etc. (I guess those would need a rooted device anyway) but used it in an application in the normal way by loading the compiler with dxc::DxcDllSupport::Initialize() etc. and all shaders compiled fine.

[jaebaek]

Should the execinfo.h include be just removed completely from WinAdapter.h since it looks like it is not really needed for non-Windows builds ATM? Or considering that the Android build works, without having to disable any target in the solution, could it be fine to just add the #ifndef ANDROID as in this pull request?

This is weird. I am not the initial author of WinAdapter.h. I am curious why we wrote the CaptureStackBackTrace macro in the file. I guess we used it in other places for Linux build, but removed them at some points. If it is only used by CompilerTest.cpp, removing the macro and the include statement (#include execinfo.h in WinAdapter.h) makes sense. Could you please test it? If it works without errors, it sounds like a better change.

[seppala2]

@jaebaek I tested this on Ubuntu, compiled all targets and ran the clang-hlsl-tests and clang-spirv-tests fine having the CaptureStackBackTrace define and execinfo.h include removed from WinAdapter.h. I can add that as a commit to this pull request if it's fine.

[jaebaek]

It sounds good to me. Please update this PR with the change.

[pow2clk]

I agree that if this include isn't needed, we should just remove it outright. That would eliminate the need to define any specific support indicator for Android and clean things up.

For the record, I didn't mean to imply that this should be implemented for Android, only that the exclusion should be done in a more maintainable way.

Also for the record, It was I who ported these tests to Linux and added that include. It looks like just a few months after, Ehsann disabled them for reasons that aren't clear to me. I suspect that is something we will want to readdress as improving testing of DXIL generation on Linux is something that has been indirectly requested by customers.

Nevertheless, go ahead and remove the include for now. It's presently useless though we may need to come up with a solution for it eventually. When that happens, we'll try not to break the Android build in the process.

[seppala2]

Ok, I added a commit that removes the execinfo.h include and CaptureStackBackTrace macro. It will be very useful to be able to build for Android. Maybe in the future if the functionality is needed again, libunwind (unwind.h) could be considered as an alternative, in case it's supported on all platforms. Seems like it's available on Mac, Linux and Android.

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.173 completed (commit 84d1bd8ab7 by @)

[jaebaek]

LGTM :)