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

[lld-macho][NFC] Refactor ObjCSelRefsSection => ObjCSelRefsHelper #86456

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

alx32
Copy link
Contributor

@alx32 alx32 commented Mar 24, 2024

In a previous PR: #83878, the intent was to make no functional changes, just refactor out the code for reuse.
However, by creating ObjCSelRefsSection as a SyntheticSection - this slightly changed the functionality of the application as the SyntheticSection constructor registers the SyntheticSection as a functional one - with an associated SyntheticInputSection.

With this change we remove this unintended consequence by making the code not use a SyntheticSection as base, but just by having it be a static helper.

Copy link

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link

✅ With the latest revision this PR passed the Python code formatter.

Copy link
Contributor

@kyulee-com kyulee-com left a comment

Choose a reason for hiding this comment

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

LGTM. It appears this was my original comment on not inheriting SyntheticSection, although I was not sure about this potential issue.
BTW, this is still a draft PR, and you might want to publish it so that everyone can see this PR.

@alx32 alx32 marked this pull request as ready for review March 24, 2024 21:29
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 24, 2024

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-macho

Author: None (alx32)

Changes

In a previous PR: #83878, the intent was to make no functional changes, just refactor out the code for reuse.
However, by creating ObjCSelRefsSection as a SyntheticSection - this slightly changed the functionality of the application as the SyntheticSection constructor registers the SyntheticSection as a functional one - with an associated SyntheticInputSection.

With this change we remove this unintended consequence by making the code not use a SyntheticSection as base, but just by having it be a static helper.


Full diff: https://github.com/llvm/llvm-project/pull/86456.diff

4 Files Affected:

  • (modified) lld/MachO/Driver.cpp (+1)
  • (modified) lld/MachO/SyntheticSections.cpp (+10-9)
  • (modified) lld/MachO/SyntheticSections.h (+6-15)
  • (modified) lld/MachO/Writer.cpp (+1-2)
diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp
index 919a14b8bcf08b..14c111ce9685c9 100644
--- a/lld/MachO/Driver.cpp
+++ b/lld/MachO/Driver.cpp
@@ -1394,6 +1394,7 @@ bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
     syntheticSections.clear();
     thunkMap.clear();
     unprocessedLCLinkerOptions.clear();
+    ObjCSelRefsHelper::cleanup();
 
     firstTLVDataSection = nullptr;
     tar = nullptr;
diff --git a/lld/MachO/SyntheticSections.cpp b/lld/MachO/SyntheticSections.cpp
index 1b3694528de1dd..0afbbd478bb9fd 100644
--- a/lld/MachO/SyntheticSections.cpp
+++ b/lld/MachO/SyntheticSections.cpp
@@ -806,10 +806,9 @@ void StubHelperSection::setUp() {
   dyldPrivate->used = true;
 }
 
-ObjCSelRefsSection::ObjCSelRefsSection()
-    : SyntheticSection(segment_names::data, section_names::objcSelrefs) {}
-
-void ObjCSelRefsSection::initialize() {
+llvm::DenseMap<llvm::CachedHashStringRef, ConcatInputSection *>
+    ObjCSelRefsHelper::methnameToSelref;
+void ObjCSelRefsHelper::initialize() {
   // Do not fold selrefs without ICF.
   if (config->icfLevel == ICFLevel::none)
     return;
@@ -836,7 +835,9 @@ void ObjCSelRefsSection::initialize() {
   }
 }
 
-ConcatInputSection *ObjCSelRefsSection::makeSelRef(StringRef methname) {
+void ObjCSelRefsHelper::cleanup() { methnameToSelref.clear(); }
+
+ConcatInputSection *ObjCSelRefsHelper::makeSelRef(StringRef methname) {
   auto methnameOffset =
       in.objcMethnameSection->getStringOffset(methname).outSecOff;
 
@@ -861,7 +862,7 @@ ConcatInputSection *ObjCSelRefsSection::makeSelRef(StringRef methname) {
   return objcSelref;
 }
 
-ConcatInputSection *ObjCSelRefsSection::getSelRef(StringRef methname) {
+ConcatInputSection *ObjCSelRefsHelper::getSelRef(StringRef methname) {
   auto it = methnameToSelref.find(CachedHashStringRef(methname));
   if (it == methnameToSelref.end())
     return nullptr;
@@ -890,8 +891,8 @@ StringRef ObjCStubsSection::getMethname(Symbol *sym) {
 void ObjCStubsSection::addEntry(Symbol *sym) {
   StringRef methname = getMethname(sym);
   // We create a selref entry for each unique methname.
-  if (!in.objcSelRefs->getSelRef(methname))
-    in.objcSelRefs->makeSelRef(methname);
+  if (!ObjCSelRefsHelper::getSelRef(methname))
+    ObjCSelRefsHelper::makeSelRef(methname);
 
   auto stubSize = config->objcStubsMode == ObjCStubsMode::fast
                       ? target->objcStubsFastSize
@@ -940,7 +941,7 @@ void ObjCStubsSection::writeTo(uint8_t *buf) const {
     Defined *sym = symbols[i];
 
     auto methname = getMethname(sym);
-    InputSection *selRef = in.objcSelRefs->getSelRef(methname);
+    InputSection *selRef = ObjCSelRefsHelper::getSelRef(methname);
     assert(selRef != nullptr && "no selref for methname");
     auto selrefAddr = selRef->getVA(0);
     target->writeObjCMsgSendStub(buf + stubOffset, sym, in.objcStubs->addr,
diff --git a/lld/MachO/SyntheticSections.h b/lld/MachO/SyntheticSections.h
index 6d85f0aea8e04c..4586a4a0bf4361 100644
--- a/lld/MachO/SyntheticSections.h
+++ b/lld/MachO/SyntheticSections.h
@@ -315,24 +315,16 @@ class StubHelperSection final : public SyntheticSection {
   Defined *dyldPrivate = nullptr;
 };
 
-class ObjCSelRefsSection final : public SyntheticSection {
+class ObjCSelRefsHelper {
 public:
-  ObjCSelRefsSection();
-  void initialize();
-
-  // This SyntheticSection does not do directly write data to the output, it is
-  // just a placeholder for easily creating SyntheticInputSection's which will
-  // be inserted into inputSections and handeled by the default writing
-  // mechanism.
-  uint64_t getSize() const override { return 0; }
-  bool isNeeded() const override { return false; }
-  void writeTo(uint8_t *buf) const override {}
+  static void initialize();
+  static void cleanup();
 
-  ConcatInputSection *getSelRef(StringRef methname);
-  ConcatInputSection *makeSelRef(StringRef methname);
+  static ConcatInputSection *getSelRef(StringRef methname);
+  static ConcatInputSection *makeSelRef(StringRef methname);
 
 private:
-  llvm::DenseMap<llvm::CachedHashStringRef, ConcatInputSection *>
+  static llvm::DenseMap<llvm::CachedHashStringRef, ConcatInputSection *>
       methnameToSelref;
 };
 
@@ -813,7 +805,6 @@ struct InStruct {
   LazyPointerSection *lazyPointers = nullptr;
   StubsSection *stubs = nullptr;
   StubHelperSection *stubHelper = nullptr;
-  ObjCSelRefsSection *objcSelRefs = nullptr;
   ObjCStubsSection *objcStubs = nullptr;
   UnwindInfoSection *unwindInfo = nullptr;
   ObjCImageInfoSection *objCImageInfo = nullptr;
diff --git a/lld/MachO/Writer.cpp b/lld/MachO/Writer.cpp
index 8f335188e12c7b..a18b5268fd42aa 100644
--- a/lld/MachO/Writer.cpp
+++ b/lld/MachO/Writer.cpp
@@ -720,7 +720,7 @@ static void addNonWeakDefinition(const Defined *defined) {
 
 void Writer::scanSymbols() {
   TimeTraceScope timeScope("Scan symbols");
-  in.objcSelRefs->initialize();
+  ObjCSelRefsHelper::initialize();
   for (Symbol *sym : symtab->getSymbols()) {
     if (auto *defined = dyn_cast<Defined>(sym)) {
       if (!defined->isLive())
@@ -1359,7 +1359,6 @@ void macho::createSyntheticSections() {
   in.got = make<GotSection>();
   in.tlvPointers = make<TlvPointerSection>();
   in.stubs = make<StubsSection>();
-  in.objcSelRefs = make<ObjCSelRefsSection>();
   in.objcStubs = make<ObjCStubsSection>();
   in.unwindInfo = makeUnwindInfoSection();
   in.objCImageInfo = make<ObjCImageInfoSection>();

@alx32 alx32 merged commit e1a003d into llvm:main Mar 25, 2024
9 checks passed
@alx32 alx32 deleted the 05_selref_refactor branch April 9, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants