This is an archive of the discontinued LLVM Phabricator instance.

[cmake] Dedup linker flag check polyfill
Needs RevisionPublic

Authored by Ericson2314 on Jul 25 2022, 8:34 PM.

Details

Reviewers
phosek
Mordante
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Summary

Finishing what https://reviews.llvm.org/D117537 started. This is also
what @phosek wanted.

We had two issues before, which are now fixed.

  1. HandleOutOfTreeLLVM didn't work with compiler-rt

    Removed in D125561
  1. CMake's CMP0056 couldn't be relied upon

    Locally forced on in D118546

Diff Detail

Event Timeline

Ericson2314 created this revision.Jul 25 2022, 8:34 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 25 2022, 8:34 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Ericson2314 requested review of this revision.Jul 25 2022, 8:34 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJul 25 2022, 8:34 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript

Retrigger CI

I wonder if we can de-duplicate the checks and re-use the results across the projects.

compiler-rt/cmake/config-ix.cmake
178–179

I really wold rather prefer -ztext as the spelling rather than -Wl,-z,text. -z flags are passed through the driver to the linker. This would mean that we match the behaviour with things like -fuse-ld=lld below.

197

Similar, -zglobal would be preferable IMO.

Ericson2314 added inline comments.Jul 28 2022, 3:46 PM
compiler-rt/cmake/config-ix.cmake
197

@compnerd there are some other occurences -Wl,-z in this file, would like them all to be plain -z?

phosek accepted this revision.Jul 29 2022, 12:03 AM

LGTM

Ericson2314 edited the summary of this revision. (Show Details)Aug 1 2022, 8:27 AM
Mordante requested changes to this revision.Sep 4 2023, 10:03 AM
Mordante added a subscriber: Mordante.

We're moving to GitHub PRs and for libc++ we like to cleanup the review queue before making Phabricator read-only. Is there still interest in this patch? If so can you rebased it and try to finish it?

This revision now requires changes to proceed.Sep 4 2023, 10:03 AM