Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

Differential D142174

[OpenMP] Don't set rpath for system paths
AbandonedPublic
Actions

Authored by tstellar on Jan 19 2023, 8:31 PM.

Details

Reviewers
JonChesterfield
MaskRay
jdoerfert
Summary

Suggested in https://reviews.llvm.org/D118493#3402001

Diff Detail

Unit TestsFailed
View All

TimeTest
3,650 mslibcxx CI Modules > llvm-libc++-shared-cfg-in.libcxx/algorithms/specialized_algorithms/special_mem_concepts::nothrow_sentinel_for.compile.pass.cpp

Event Timeline

tstellar created this revision.Jan 19 2023, 8:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 8:31 PM
Herald added subscribers: guansong, yaxunl. · View Herald Transcript
tstellar requested review of this revision.Jan 19 2023, 8:31 PM
Herald added a reviewer: jdoerfert. · View Herald TranscriptJan 19 2023, 8:31 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: cfe-commits, sstefan1. · View Herald Transcript
tstellar added inline comments.Jan 19 2023, 8:32 PM
clang/lib/Driver/ToolChains/CommonArgs.cpp
799

I wasn't sure exactly how to determine if something is a system path, so I'm open to other ideas here.

Harbormaster completed remote builds in B208893: Diff 490707.Jan 19 2023, 9:16 PM
MaskRay mentioned this in D118493: Set rpath on openmp executables.Jan 23 2023, 8:35 PM
JonChesterfield accepted this revision.Mar 6 2023, 8:09 AM

I'm happy with this but agree that "what might be a system path?" is a tricky heuristic. What we want is to exclude the places that the application will search anyway, but that's OS dependent. I'm mostly worried about toolchains installed under /opt or under $HOME so this is good for me. It's also easy to pull later if we reconsider.

This revision is now accepted and ready to land.Mar 6 2023, 8:09 AM
MaskRay added a comment.Mar 6 2023, 2:16 PM
In D142174#4171731, @JonChesterfield wrote:

I'm happy with this but agree that "what might be a system path?" is a tricky heuristic. What we want is to exclude the places that the application will search anyway, but that's OS dependent. I'm mostly worried about toolchains installed under /opt or under $HOME so this is good for me. It's also easy to pull later if we reconsider.

Thank you for being considerate the use case. But I don't think we should base the decision on whether /usr/lib or /usr/lib64 is used and make directories different. We should unconditionally disable rpath, regardless of the directory path.

tstellar abandoned this revision.Mar 9 2023, 9:44 PM

-fopenmp-add-rpath has been removed.

Revision Contents
Changeset List

PathSize
clang/
lib/
Driver/
ToolChains/
12 lines

Diff 490707

View Options

clang/lib/Driver/ToolChains/CommonArgs.cpp

Loading...