Skip to content

[LLD][COFF] Introduce Symbol::getDefined helper. (NFC) #151253

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

Merged
merged 1 commit into from
Jul 31, 2025

Conversation

cjacek
Copy link
Contributor

@cjacek cjacek commented Jul 29, 2025

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Jul 29, 2025

@llvm/pr-subscribers-platform-windows
@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-coff

Author: Jacek Caban (cjacek)

Changes

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

3 Files Affected:

  • (modified) lld/COFF/Driver.cpp (+2-10)
  • (modified) lld/COFF/Symbols.cpp (+8)
  • (modified) lld/COFF/Symbols.h (+4)
diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index 570b8f9d05906..192a998229e92 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -1318,13 +1318,9 @@ void LinkerDriver::convertResources() {
 }
 
 void LinkerDriver::maybeCreateECExportThunk(StringRef name, Symbol *&sym) {
-  Defined *def;
   if (!sym)
     return;
-  if (auto undef = dyn_cast<Undefined>(sym))
-    def = undef->getDefinedWeakAlias();
-  else
-    def = dyn_cast<Defined>(sym);
+  Defined *def = sym->getDefined();
   if (!def)
     return;
 
@@ -1356,11 +1352,7 @@ void LinkerDriver::createECExportThunks() {
     Symbol *sym = ctx.symtab.find(targetName);
     if (!sym)
       continue;
-    Defined *targetSym;
-    if (auto undef = dyn_cast<Undefined>(sym))
-      targetSym = undef->getDefinedWeakAlias();
-    else
-      targetSym = dyn_cast<Defined>(sym);
+    Defined *targetSym = sym->getDefined();
     if (!targetSym)
       continue;
 
diff --git a/lld/COFF/Symbols.cpp b/lld/COFF/Symbols.cpp
index b571ce9ce432c..ba4f95d14bc64 100644
--- a/lld/COFF/Symbols.cpp
+++ b/lld/COFF/Symbols.cpp
@@ -91,6 +91,14 @@ bool Symbol::isLive() const {
   return true;
 }
 
+Defined *Symbol::getDefined() {
+  if (auto d = dyn_cast<Defined>(this))
+    return d;
+  if (auto u = dyn_cast<Undefined>(this))
+    return u->getDefinedWeakAlias();
+  return nullptr;
+}
+
 void Symbol::replaceKeepingName(Symbol *other, size_t size) {
   StringRef origName = getName();
   memcpy(this, other, size);
diff --git a/lld/COFF/Symbols.h b/lld/COFF/Symbols.h
index fd3d8cec8c113..c86ded860876b 100644
--- a/lld/COFF/Symbols.h
+++ b/lld/COFF/Symbols.h
@@ -95,6 +95,10 @@ class Symbol {
            symbolKind == LazyDLLSymbolKind;
   }
 
+  // Get the Defined symbol associated with this symbol, either itself or its
+  // weak alias.
+  Defined *getDefined();
+
 private:
   void computeName();
 

@cjacek
Copy link
Contributor Author

cjacek commented Jul 29, 2025

A small cleanup, split from a larger -arm64xsameaddress support patch, which will use it in more places.

Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

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

LGTM with a discussion comment.

Defined *Symbol::getDefined() {
if (auto d = dyn_cast<Defined>(this))
return d;
if (auto u = dyn_cast<Undefined>(this))
Copy link
Member

Choose a reason for hiding this comment

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

This (very) vaguely changes the logic from before - previously we tested dyn_cast<Undefined> before, while we now test them in different order. (In theory this could have a performance aspect, but that's not relevant.) What I wanted to say here, is that as long as only one of dyn_cast<Defined> and dyn_cast<Undefined> can succeed, i.e. they are mutually exclusive, then this should be fine - and this mildly rewritten form looks ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I believe the assumption that only one of them can succeed is safe. I wrote it in this order because I’d expect Defined to be more common, but as you said, the performance impact is not really relevant.

@cjacek cjacek merged commit aac70d6 into llvm:main Jul 31, 2025
13 checks passed
@cjacek cjacek deleted the sym-get-defined branch July 31, 2025 10:58
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.

3 participants