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

[R] [8.1] No more COPY_HEADERS, default c++ std, CleanSpec, cleanups #29

Merged
merged 6 commits into from
Sep 10, 2020

Conversation

ix5
Copy link

@ix5 ix5 commented Apr 29, 2020

CleanSpec: Clean system/vendor as well

CleanSpec: Fix copied headers path

Fix the removed path so that both obj/ and obj_arm/ are cleared.

This ensures that we do not make mistakes when porting away from LOCAL_COPY_HEADERS and get bitten when we finally get around to doing a clean build.

treewide: Cleanup includes, include logic

  • Android.bp: subdirs is deprecated and unnceeded in Q.
  • Top-level Android.mk: Organize for better readability, account for platforms named sdm in addition to msm.
  • Treewide: Remove empty common_includes.

Remove reliance on COPY_HEADERS

LOCAL_COPY_HEADERS is deprecated and will throw a build error in Android R.

The header includes are duplicated anyway, the display_headers module already exports all necessary headers.

sdm: Fix invalid string memory access

CAF doesn't get to decide what std version to use

MarijnS95 and others added 6 commits May 1, 2020 21:29
Change-Id: I4047a18879d41d6f18a6f6b266fdc773e09ea0a6
LOCAL_COPY_HEADERS is deprecated and will throw a build
error in Android R.

The header includes are duplicated anyway, the
`display_headers` module already exports all necessary
headers.
Android.bp: `subdirs` is deprecated and unnceeded in Q.

Top-level Android.mk: Organize for better readability,
account for platforms named `sdm` in addition to `msm`.

Treewide: Remove empty `common_includes`.
Fix the removed path so that both obj/ and obj_arm/ are
cleared.

This ensures that we do not make mistakes when porting away
from LOCAL_COPY_HEADERS and get bitten when we finally get
around to doing a clean build.
@ix5 ix5 force-pushed the 81r1-no-copy-headers branch from b56f72b to 19ebe63 Compare May 1, 2020 19:32
@ix5 ix5 changed the title [DONOTMERGE] [R] [8.1] Remove reliance on COPY_HEADERS [DONOTMERGE] [R] [8.1] No more COPY_HEADERS, default c++ std, CleanSpec, cleanups May 1, 2020
@ix5 ix5 marked this pull request as ready for review May 1, 2020 23:00
@oshmoun
Copy link
Contributor

oshmoun commented May 2, 2020

0d48278
not sure I understand how this changes anything

@MarijnS95
Copy link
Contributor

MarijnS95 commented Jun 5, 2020

@oshmoun For completeness let me write the explanation here too :)

char * is just a pointer to some memory, std::string is a string object that owns that piece of memory. Getting a pointer to that piece of memory (.c_str()) and then destroying the std::string (because it goes out of scope) means that memory is freed and .c_str() now points to invalid memory. The string may still be there, it may be garbage, or the entire page it's in might be reclaimed resulting in a nice segfault.

The correct way is to only ever return pointers to static readonly strings (eg. a character literal like "foo") or the managed std::string, which is what happens here. .c_str() is only taken and used while backing memory is still in scope.

@MarijnS95
Copy link
Contributor

Also, this seems to build totally fine on Q too (for Akatsuki). And yes, I ran rm out/target/product/*/obj/include/ -r this time 😉.

@MarijnS95
Copy link
Contributor

I have been using this on Q for since the PR was submitted... @jerpelea MERGE!

@jerpelea jerpelea merged commit 7998e72 into sonyxperiadev:aosp/LA.UM.8.1.r1 Sep 10, 2020
@ix5 ix5 deleted the 81r1-no-copy-headers branch September 11, 2020 16:13
@ix5 ix5 changed the title [DONOTMERGE] [R] [8.1] No more COPY_HEADERS, default c++ std, CleanSpec, cleanups [R] [8.1] No more COPY_HEADERS, default c++ std, CleanSpec, cleanups Feb 1, 2021
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