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

build: update clang-fetcher with libcxx headers and object paths #703

Closed
wants to merge 3 commits into from
Closed

build: update clang-fetcher with libcxx headers and object paths #703

wants to merge 3 commits into from

Conversation

VerteDinde
Copy link
Member

The accompanying PR to 29281 in Electron.

Beginning in Electron 13, several nan tests began failing due to an incompatibility between libc++ and libstdc++. This, in turn, could affect any native dependencies built by users using nan. We will now build these native dependencies against libc++ to fix this and prevent future incompatibilities, by copying some of the relevant build arguments from Electron into our nan test runner.

Electron 13 and up now build libc++ and libc++abi as static libraries for Mac and Linux. The relevant zipped headers for these libraries are also checked and uploaded per release - this PR updates electron-rebuild with these build changes, and use the zipped headers to apply them.

TODO:

  • Test that header path is correct
  • Add tests

@codecov-commenter
Copy link

codecov-commenter commented May 25, 2021

Codecov Report

Merging #703 (ea37945) into master (c35888a) will decrease coverage by 3.89%.
The diff coverage is 24.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #703      +/-   ##
==========================================
- Coverage   69.39%   65.50%   -3.90%     
==========================================
  Files          11       12       +1     
  Lines         575      629      +54     
  Branches      118      125       +7     
==========================================
+ Hits          399      412      +13     
- Misses        135      175      +40     
- Partials       41       42       +1     
Impacted Files Coverage Δ
src/libcxx-fetcher.ts 23.80% <23.80%> (ø)
src/clang-fetcher.ts 73.17% <25.00%> (-8.26%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c35888a...ea37945. Read the comment docs.

package.json Show resolved Hide resolved

const cxxflags = [
'-nostdinc++',
'-D_LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: with roll electron/electron#29895 , this macro should be removed in favor of including the __config_site file from the root of libcxx header file. electron/electron@797723e

@deepak1556
Copy link
Member

Another FYI: libcxx objects built for testing and release are different in terms of the compiler and linker flags used, specifically on linux ThinLTO is used. If we don't use the lto flags https://source.chromium.org/chromium/chromium/src/+/main:build/config/compiler/BUILD.gn;l=640-727 to build the native module then miscompile can happen. For ex, compile the following program with the compile and linker flags from this PR, the result will be zero due to miscompilation. Originally caught in microsoft/vscode#129360, I would suggest to find a concrete way to export compiler flags from electron build, probably bundle libc++.ninja, libc++abi.ninja as part of libcxx_objects and this module can extract the values from the file.

#include <string>
#include <sstream>

int main() {
    std::string message = "Hello";
    uint32_t count = 0;

    std::stringstream final_value;
    final_value << count;
    final_value.write(message.data(), message.size());

    printf("Result : %lu", final_value.str().size());

    return 0;
}

@VerteDinde
Copy link
Member Author

Taking another stab at this in light of #832, may close this PR in favor of a new one.

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