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

Using precompiled header causes compilation error of tagged files #14

Closed
DoDoENT opened this issue Mar 14, 2016 · 5 comments
Closed

Using precompiled header causes compilation error of tagged files #14

DoDoENT opened this issue Mar 14, 2016 · 5 comments
Assignees
Milestone

Comments

@DoDoENT
Copy link

DoDoENT commented Mar 14, 2016

Problem is that precompiled header is built only for non-tagged source files and all files attempt to use it. So when a specific file is tagged with arm or neon tag, then this file cannot use PCH and compile warning is issued. When all warnings are treated as errors, this causes compilation failure.

This problem was also present in r10e.

@DoDoENT
Copy link
Author

DoDoENT commented Mar 14, 2016

Here is my patch which workarounds the issue by disabling usage of PCH for all files that have tags:

diff  orig/build-binary.mk build-binary.mk 
443,444c443,454
<     # All obj files are dependent on the PCH
<     $(foreach src,$(filter $(all_cpp_patterns),$(LOCAL_SRC_FILES)),\
---
>      # Filter obj files that are dependent on the PCH (only those without tags)
>     ifeq (true,$(LOCAL_ARM_NEON))
>         TAGS_TO_FILTER=arm
>     else
>         TAGS_TO_FILTER=arm neon
>     endif
> 
>     allowed_src := $(foreach src,$(filter $(all_cpp_patterns),$(LOCAL_SRC_FILES)),\
>         $(if $(filter $(TAGS_TO_FILTER),$(LOCAL_SRC_FILES_TAGS.$(src))),,$(src))\
>     )
>     # All files without tags depend on PCH
>     $(foreach src,$(allowed_src),\
447,449c457,458
< 
<     # Files from now on build with PCH
<     LOCAL_CPPFLAGS += -Winvalid-pch -include $(LOCAL_OBJS_DIR)/$(LOCAL_BUILT_PCH)
---
>     # Make sure those files are built with PCH
>     $(call add-src-files-target-cflags,$(allowed_src),-Winvalid-pch -include $(LOCAL_OBJS_DIR)/$(LOCAL_BUILT_PCH))

@DanAlbert
Copy link
Member

Thanks for the patch. I added a test case and uploaded for review:https://android-review.googlesource.com/207977

@DanAlbert DanAlbert self-assigned this Mar 14, 2016
@DanAlbert DanAlbert added this to the r12 milestone Mar 14, 2016
@DoDoENT
Copy link
Author

DoDoENT commented Mar 15, 2016

@DanAlbert, thank you for accepting the patch. I would only like to note that this patch works by disabling PCH for tagged files (i.e. files tagged with arm or neon will not use PCH). While this works fine for our use case when such files are minority, the real fix would actually be to create a separate PCH for each tag combination in use (I believe this is what Xcode does when compiling iOS projects).

@DanAlbert
Copy link
Member

Yeah, I had considered that. There are bigger problems to chase right now, so I figure something is better than nothing :)

One thing that can be done to avoid this is to use a separate (intermediate) static library for the sources that would normally be tagged. This way you can set LOCAL_ARM_MODE and LOCAL_ARM_NEON correctly for each static lib and the LOCAL_PCH will automatically be built with the right flags for each lib. Then you just use them via LOCAL_WHOLE_STATIC_LIBRARIES to ensure you get the full library included.

@DoDoENT
Copy link
Author

DoDoENT commented Mar 16, 2016

Thanks @DanAlbert, that is a good idea.

DanAlbert added a commit that referenced this issue Apr 16, 2016
Still generate the one precompiled header for the default
configuration, but don't use it for sources that have additional tags
(.arm or .neon, shouldn't affect non-ARM32).

Patch contributed by @DoDoENT.

Bug: #14
Change-Id: I1fff1599b3f30e69134aed4972117745bc9c053a
@PvR33 PvR33 mentioned this issue Sep 18, 2023
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

No branches or pull requests

2 participants