Skip to content

[Object][COFF][NFC] Don't use inline function for COFFImportFile::printSymbolName. #87195

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
Mar 31, 2024

Conversation

cjacek
Copy link
Contributor

@cjacek cjacek commented Mar 31, 2024

Fixes BUILD_SHARED_LIBS builds after #87191 made helpers non-inline.

…ntSymbolName.

Fixes BUILD_SHARED_LIBS builds after llvm#87191 made helpers non-inline.
@llvmbot
Copy link
Member

llvmbot commented Mar 31, 2024

@llvm/pr-subscribers-llvm-binary-utilities

Author: Jacek Caban (cjacek)

Changes

Fixes BUILD_SHARED_LIBS builds after #87191 made helpers non-inline.


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

2 Files Affected:

  • (modified) llvm/include/llvm/Object/COFFImportFile.h (+1-20)
  • (modified) llvm/lib/Object/COFFImportFile.cpp (+21)
diff --git a/llvm/include/llvm/Object/COFFImportFile.h b/llvm/include/llvm/Object/COFFImportFile.h
index 035bc17126da34..2c06f529ecdfc5 100644
--- a/llvm/include/llvm/Object/COFFImportFile.h
+++ b/llvm/include/llvm/Object/COFFImportFile.h
@@ -45,26 +45,7 @@ class COFFImportFile : public SymbolicFile {
 
   void moveSymbolNext(DataRefImpl &Symb) const override { ++Symb.p; }
 
-  Error printSymbolName(raw_ostream &OS, DataRefImpl Symb) const override {
-    switch (Symb.p) {
-    case ImpSymbol:
-      OS << "__imp_";
-      break;
-    case ECAuxSymbol:
-      OS << "__imp_aux_";
-      break;
-    }
-    const char *Name = Data.getBufferStart() + sizeof(coff_import_header);
-    if (Symb.p != ECThunkSymbol && COFF::isArm64EC(getMachine())) {
-      if (std::optional<std::string> DemangledName =
-              getArm64ECDemangledFunctionName(Name)) {
-        OS << StringRef(*DemangledName);
-        return Error::success();
-      }
-    }
-    OS << StringRef(Name);
-    return Error::success();
-  }
+  Error printSymbolName(raw_ostream &OS, DataRefImpl Symb) const override;
 
   Expected<uint32_t> getSymbolFlags(DataRefImpl Symb) const override {
     return SymbolRef::SF_Global;
diff --git a/llvm/lib/Object/COFFImportFile.cpp b/llvm/lib/Object/COFFImportFile.cpp
index 477c5bf98249f7..48c3ea0ed8f4e4 100644
--- a/llvm/lib/Object/COFFImportFile.cpp
+++ b/llvm/lib/Object/COFFImportFile.cpp
@@ -84,6 +84,27 @@ StringRef COFFImportFile::getExportName() const {
   return name;
 }
 
+Error COFFImportFile::printSymbolName(raw_ostream &OS, DataRefImpl Symb) const {
+  switch (Symb.p) {
+  case ImpSymbol:
+    OS << "__imp_";
+    break;
+  case ECAuxSymbol:
+    OS << "__imp_aux_";
+    break;
+  }
+  const char *Name = Data.getBufferStart() + sizeof(coff_import_header);
+  if (Symb.p != ECThunkSymbol && COFF::isArm64EC(getMachine())) {
+    if (std::optional<std::string> DemangledName =
+            getArm64ECDemangledFunctionName(Name)) {
+      OS << StringRef(*DemangledName);
+      return Error::success();
+    }
+  }
+  OS << StringRef(Name);
+  return Error::success();
+}
+
 static uint16_t getImgRelRelocation(MachineTypes Machine) {
   switch (Machine) {
   default:

@cjacek cjacek merged commit 8d8fff0 into llvm:main Mar 31, 2024
@dwblaikie
Copy link
Collaborator

This was sent for review but review wasn't completed? (if it was not intended for review, only for precommit automated testing, please mark such changes in the future as skip-precommit-approval label ( https://llvm.org/docs/CodeReview.html#code-review-workflow ))

But also this sort of change seems suspect. It sounds like some dependency/layering isn't properly described? It shouldn't be necessary/correct/make any difference to move implementations between headers and implementation files if we've described the dependencies correctly - and if we haven't, we should fix that, because living in a world where code can't move freely between header and cpp file is a maintenance problem.

@cjacek
Copy link
Contributor Author

cjacek commented Apr 1, 2024

I should add skip-precommit-approval, I'm sorry about that. I intended to fix buildbot issue quickly and it seemed simple enough for a post-commit review. Sorry if I misjudged, I'm happy to address any feedback.

It's about getArm64ECDemangledFunctionName function being used in a header, that previously was inline too. This made any user of COFFImportFile.h require to explicitly link to the dependent Core library and that was not the case for llvm-readobj. Object library (where COFFImportFile.cpp is implemented) already links to Core, so it seems fine to me. I think that for similar reasons a lot of code can't be freely moved between cpp and header files.

@cjacek cjacek deleted the implib-print-name branch April 9, 2024 11:06
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