[llvm] 9a62d9d - [JITLink][MachO] Fix alignment bug in the c-string literal section graphifier.

Lang Hames via llvm-commits llvm-commits at lists.llvm.org
Tue Apr 5 17:39:00 PDT 2022


Author: Lang Hames
Date: 2022-04-05T17:38:54-07:00
New Revision: 9a62d9db2e1f6064e6c20344870f5e9d43a8de43

URL: https://github.com/llvm/llvm-project/commit/9a62d9db2e1f6064e6c20344870f5e9d43a8de43
DIFF: https://github.com/llvm/llvm-project/commit/9a62d9db2e1f6064e6c20344870f5e9d43a8de43.diff

LOG: [JITLink][MachO] Fix alignment bug in the c-string literal section graphifier.

This function had been assuming a 1-byte alignment, which isn't always correct.
This commit updates it to take the alignment from the __cstring section.

The key change is to the createContentBlock call, but the surrounding code is
updated with clearer debugging output to support the testcase (and any future
debugging work).

Added: 
    llvm/test/ExecutionEngine/JITLink/X86/MachO_cstring_section_alignment.s

Modified: 
    llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h
    llvm/lib/ExecutionEngine/JITLink/MachOLinkGraphBuilder.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h b/llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h
index 70f7fb45b39e7..897808c0ee835 100644
--- a/llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h
+++ b/llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h
@@ -223,6 +223,11 @@ class Block : public Addressable {
   /// Returns the size of this defined addressable.
   size_t getSize() const { return Size; }
 
+  /// Returns the address range of this defined addressable.
+  orc::ExecutorAddrRange getRange() const {
+    return orc::ExecutorAddrRange(getAddress(), getSize());
+  }
+
   /// Get the content for this block. Block must not be a zero-fill block.
   ArrayRef<char> getContent() const {
     assert(Data && "Block does not contain content");
@@ -576,6 +581,11 @@ class Symbol {
     this->Size = Size;
   }
 
+  /// Returns the address range of this symbol.
+  orc::ExecutorAddrRange getRange() const {
+    return orc::ExecutorAddrRange(getAddress(), getSize());
+  }
+
   /// Returns true if this symbol is backed by a zero-fill block.
   /// This method may only be called on defined symbols.
   bool isSymbolZeroFill() const { return getBlock().isZeroFill(); }

diff  --git a/llvm/lib/ExecutionEngine/JITLink/MachOLinkGraphBuilder.cpp b/llvm/lib/ExecutionEngine/JITLink/MachOLinkGraphBuilder.cpp
index 0475117eec39c..1bf12f438be07 100644
--- a/llvm/lib/ExecutionEngine/JITLink/MachOLinkGraphBuilder.cpp
+++ b/llvm/lib/ExecutionEngine/JITLink/MachOLinkGraphBuilder.cpp
@@ -644,17 +644,27 @@ Error MachOLinkGraphBuilder::graphifyCStringSection(
   // Scan section for null characters.
   for (size_t I = 0; I != NSec.Size; ++I)
     if (NSec.Data[I] == '\0') {
-      orc::ExecutorAddrDiff BlockEnd = I + 1;
-      size_t BlockSize = BlockEnd - BlockStart;
+      size_t BlockSize = I + 1 - BlockStart;
       // Create a block for this null terminated string.
       auto &B = G->createContentBlock(*NSec.GraphSection,
                                       {NSec.Data + BlockStart, BlockSize},
-                                      NSec.Address + BlockStart, 1, 0);
+                                      NSec.Address + BlockStart, NSec.Alignment,
+                                      BlockStart % NSec.Alignment);
 
       LLVM_DEBUG({
-        dbgs() << "    Created block " << formatv("{0:x}", B.getAddress())
-               << " -- " << formatv("{0:x}", B.getAddress() + B.getSize())
-               << " for \"" << StringRef(B.getContent().data()) << "\"\n";
+        dbgs() << "    Created block " << B.getRange()
+               << ", align = " << B.getAlignment()
+               << ", align-ofs = " << B.getAlignmentOffset() << " for \"";
+        for (size_t J = 0; J != std::min(B.getSize(), size_t(16)); ++J)
+          switch (B.getContent()[J]) {
+          case '\0': break;
+          case '\n': dbgs() << "\\n"; break;
+          case '\t': dbgs() << "\\t"; break;
+          default:   dbgs() << B.getContent()[J]; break;
+          }
+        if (B.getSize() > 16)
+          dbgs() << "...";
+        dbgs() << "\"\n";
       });
 
       // If there's no symbol at the start of this block then create one.
@@ -663,15 +673,13 @@ Error MachOLinkGraphBuilder::graphifyCStringSection(
         auto &S = G->addAnonymousSymbol(B, 0, BlockSize, false, false);
         setCanonicalSymbol(NSec, S);
         LLVM_DEBUG({
-          dbgs() << "      Adding anonymous symbol for c-string block "
-                 << formatv("{0:x16} -- {1:x16}", S.getAddress(),
-                            S.getAddress() + BlockSize)
-                 << "\n";
+          dbgs() << "      Adding symbol for c-string block " << B.getRange()
+                 << ": <anonymous symbol> at offset 0\n";
         });
       }
 
       // Process any remaining symbols that point into this block.
-      auto LastCanonicalAddr = B.getAddress() + BlockEnd;
+      auto LastCanonicalAddr = B.getAddress() + BlockSize;
       while (!NSyms.empty() && orc::ExecutorAddr(NSyms.back()->Value) <
                                    B.getAddress() + BlockSize) {
         auto &NSym = *NSyms.back();
@@ -686,8 +694,15 @@ Error MachOLinkGraphBuilder::graphifyCStringSection(
           LastCanonicalAddr = orc::ExecutorAddr(NSym.Value);
         }
 
-        createStandardGraphSymbol(NSym, B, SymSize, SectionIsText, SymLive,
-                                  IsCanonical);
+        auto &Sym = createStandardGraphSymbol(NSym, B, SymSize, SectionIsText,
+                                              SymLive, IsCanonical);
+        (void)Sym;
+        LLVM_DEBUG({
+          dbgs() << "      Adding symbol for c-string block " << B.getRange()
+                 << ": "
+                 << (Sym.hasName() ? Sym.getName() : "<anonymous symbol>")
+                 << " at offset " << formatv("{0:x}", Sym.getOffset()) << "\n";
+        });
 
         NSyms.pop_back();
       }

diff  --git a/llvm/test/ExecutionEngine/JITLink/X86/MachO_cstring_section_alignment.s b/llvm/test/ExecutionEngine/JITLink/X86/MachO_cstring_section_alignment.s
new file mode 100644
index 0000000000000..5a8cef5749f0a
--- /dev/null
+++ b/llvm/test/ExecutionEngine/JITLink/X86/MachO_cstring_section_alignment.s
@@ -0,0 +1,26 @@
+# REQUIRES: asserts
+# RUN: llvm-mc -triple=x86_64-apple-macos10.9 -filetype=obj -o %t %s
+# RUN: llvm-jitlink -debug-only=jitlink -noexec %t 2>&1 | FileCheck %s
+#
+# Verify that PC-begin candidate symbols have been sorted correctly when adding
+# PC-begin edges for FDEs. In this test both _main and _X are at address zero,
+# however we expect to select _main over _X as _X is common. If the sorting
+# fails we'll trigger an assert in EHFrameEdgeFixer, otherwise this test will
+# succeed.
+#
+# CHECK: Graphifying C-string literal section __TEXT,__cstring
+# CHECK:    Created block {{.*}} -- {{.*}}, align = 16, align-ofs = 0 for "abcdefghijklmno"
+
+	.section	__TEXT,__text,regular,pure_instructions
+	.build_version macos, 12, 0
+	.globl	_main
+	.p2align	4, 0x90
+_main:
+	retq
+
+	.section	__TEXT,__cstring,cstring_literals
+	.p2align	4
+L_.str.1:
+	.asciz	"abcdefghijklmno"
+
+.subsections_via_symbols


        


More information about the llvm-commits mailing list