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.

Sign up for GitHub

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

Jump to bottom

[cling] Fix build with MacOSX15.0.sdk #15900

Merged
merged 2 commits into from
Jun 24, 2024
Merged

[cling] Fix build with MacOSX15.0.sdk #15900

merged 2 commits into from
Jun 24, 2024

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Jun 21, 2024

Remove some headers from std_darwin.modulemap and backport llvm/llvm-project@09ec000 to fix the build of std.pcm.

This fixes the build but still has five failing tests:

The following tests FAILED:
        206 - gtest-math-mathcore-test-CladDerivatorTests (Failed)
        232 - gtest-roofit-histfactory-test-testHistFactory (Failed)
        235 - gtest-roofit-hs3-test-testRooFitHS3 (Failed)
        267 - gtest-roofit-roofitcore-test-testRooFuncWrapper (Failed)
        1729 - roottest-root-io-compression-make (Failed)

There seem to be at least two problems: an UNREACHABLE executed

builtin functions are handled elsewhere
UNREACHABLE executed at /Users/sftnight/jonas/root.src/interpreter/llvm-project/clang/lib/CodeGen/CGExprScalar.cpp:2036!

and Clad not being able to differentiate __builtin_pow:

[ RUN      ] CladDerivator.power
In module 'std' imported from input_line_1:1:
/Library/Developer/CommandLineTools/SDKs/MacOSX15.0.sdk/usr/include/c++/v1/__math/exponential_functions.h:162:55: warning: Unsupported declaration
  using __result_type = typename __promote<_A1, _A2>::type;
                                                      ^
/Library/Developer/CommandLineTools/SDKs/MacOSX15.0.sdk/usr/include/c++/v1/__math/exponential_functions.h:153:10: warning: function '__builtin_pow' was not differentiated because clad failed to d
ifferentiate it and no suitable overload was found in namespace 'custom_derivatives', and function may not be eligible for numerical differentiation.
  return __builtin_pow(__x, __y);
         ^
/Users/sftnight/jonas/root.src/math/mathcore/test/CladDerivatorTests.cxx:43: Failure
Expected equality of these values:
  48
  value->GetAsDouble()
    Which is: 0
[  FAILED  ] CladDerivator.power (6 ms)

I'm still investigating, starting with the first.

@hahnjo
std_darwin.modulemap: Remove headers for MacOSX15.0.sdk
78f8f29 
Manually curating the modulemap is far from ideal because it requires
updates for changes in the libc++ library shipped with the SDK, which
must also work across all supported SDK versions. An alternative would
be to locate the modulemap shipped with libc++ during configuration
time, copy it and dynamically modify its contents to suit our needs.
@hahnjo hahnjo self-assigned this Jun 21, 2024
@vgvassilev
Copy link
Member

@PetroZarytskyi, can you help out with the Clad issue here? We probably fail to find the propagator for __builtin_pow.

cc: @vaithak.

@hahnjo
Copy link
Member Author

hahnjo commented Jun 21, 2024

It seems the UNREACHABLE is connected to the issue regarding __builtin_pow, at least that is the builtin function that Clad's generated code tries to cast into a function pointer. The failure of roottest-root-io-compression-make was because I was building without builtin_zstd and the compressed size was different than expected.

From that point of view, the build fixes are complete. Note that the backport still needs to be synchronized to https://github.com/root-project/llvm-project/ before merging.

@hahnjo hahnjo marked this pull request as ready for review June 21, 2024 08:26
vgvassilev
vgvassilev approved these changes Jun 21, 2024
View reviewed changes
Copy link
Member

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

Lgtm!

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 21, 2024 •
edited
Loading

Test Results

    13 files      13 suites   2d 21h 44m 40s ⏱️
 2 646 tests  2 646 ✅ 0 💤 0 ❌
32 587 runs  32 587 ✅ 0 💤 0 ❌

Results for commit fbc9348.

♻️ This comment has been updated with latest results.

@ian-twilightcoder @hahnjo
[Modules] textual headers in submodules never resolve their uses
fbc9348 
When an include from a textual header is resolved, the textual header's
submodule is used as the requesting module. The submodule's uses are
resolved, but that doesn't work because only top level modules have
uses, and only the top level module uses are used for checking uses in
Module::directlyUses. ModuleMap::resolveUses to resolve the top level
module instead of the submodule.

---

This fixes the build of std.pcm with MacOSX15.0.sdk.
@dpiparo
Copy link
Member

dpiparo commented Jun 21, 2024

The failures are unrelated and most likely due to a glitch in the EOS based file server.

@dpiparo
Copy link
Member

dpiparo commented Jun 23, 2024

I propose to:

  1. Disable CLAD on macbeta in master and merge this PR
  2. Port this PR to 6.32, disable CLAD on macbeta and merge the PR
  3. If the work is little, backport to 6.28 and 6.30 and disable clad. Otherwise, disable macbeta builds on 6.28 and 6.30

OK with everybody @hahnjo @vgvassilev ?

@dpiparo
Copy link
Member

dpiparo commented Jun 23, 2024

Irrespective of the plan above or any modification to it, I believe we should sit down and understand how to deploy an automated way to build the module map. For example, internal headers will always be changed and it will be harder and harder to keep present versions of the sdk working and fix new ones.

@vgvassilev
Copy link
Member

I propose to:

  1. Disable CLAD on macbeta in master and merge this PR
  2. Port this PR to 6.32, disable CLAD on macbeta and merge the PR
  3. If the work is little, backport to 6.28 and 6.30 and disable clad. Otherwise, disable macbeta builds on 6.28 and 6.30

OK with everybody @hahnjo @vgvassilev ?

Makes sense. Could you also open a bug report and assign me on it?

@hahnjo hahnjo merged commit bb9629d into root-project:master Jun 24, 2024
19 checks passed
@hahnjo hahnjo deleted the macos15 branch June 24, 2024 06:18
@hahnjo
Copy link
Member Author

hahnjo commented Jun 24, 2024 •
edited
Loading

I propose to:

  1. Disable CLAD on macbeta in master and merge this PR

I decided to merge this PR as-is; clad=OFF for mac-beta is done in #15910

  1. Port this PR to 6.32, disable CLAD on macbeta and merge the PR

Agreed; let's wait for the PR above to be merged and then we can backport the two (edit: #15911).

  1. If the work is little, backport to 6.28 and 6.30 and disable clad. Otherwise, disable macbeta builds on 6.28 and 6.30

We'll need to test how LLVM 13 in general behaves for the new SDK. I can hopefully do this today.

This was referenced Jun 24, 2024
Merged
Closed
@hahnjo
Copy link
Member Author

hahnjo commented Jun 24, 2024

Could you also open a bug report and assign me on it?

Done: #15912

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devajithvs devajithvs devajithvs approved these changes

@dpiparo dpiparo dpiparo approved these changes

@vgvassilev vgvassilev vgvassilev approved these changes

Assignees

@hahnjo hahnjo

Labels
in:Cling
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hahnjo @vgvassilev @dpiparo @devajithvs @ian-twilightcoder