[lld-macho][nfc] define command UNWIND_MODE_MASK for convenience and rewrite mode-mask checking logic for clarity

[oontvoo]

Summary

Rewrite the checking logic in InputFiles.cpp for clarity.
The previous form is currently "harmless" and happened to work but may not in the future:

Consider the struct: (for x86-64, but same issue can be said for the ARM/64 families):

UNWIND_X86_64_MODE_MASK                    = 0x0F000000,
UNWIND_X86_64_MODE_RBP_FRAME               = 0x01000000,
UNWIND_X86_64_MODE_STACK_IMMD              = 0x02000000,
UNWIND_X86_64_MODE_STACK_IND               = 0x03000000,
UNWIND_X86_64_MODE_DWARF                   = 0x04000000,

Previously, we were doing: (encoding & MODE_DWARF) == MODE_DWARF

As soon as a new UNWIND_X86_64_MODE_FOO = 0x05000000 is defined, then the check above would always return true for encoding=MODE_FOO (because (0b0101 & 0b0100) == 0b0100 )

[oontvoo]

moved static_assert to outside the loop

[oontvoo]

added missing include

[int3]

Please put the "drive-by cleanup" in a separate diff... it would have made it a lot easier to figure out what this diff is doing

because there's no 0x05000000 in the enum set, but as soon as it's defined, it'll stop working

I can see why there might be an issue if 0x11000000 was a defined mode, but I don't think 0x05000000 would be a problem?

lld/MachO/InputFiles.cpp

1072–1075

can we hoist to the top level scope and then define a constexpr UNWIND_MODE_MASK = ...?

then we can use in it canFoldEncoding too (and remove the first static_assert() in that function)

lld/MachO/UnwindInfoSection.cpp

423–426

Unless there's an obvious runtime issue, I would rather we keep things the way they are. Is there any observable behavioral difference with this change?

DWARF mode encodings will almost (always?) never get folded anyway since each one includes an offset to the corresponding EH frame. And since we don't do ICF on EH frames, they should all be distinct...

[oontvoo]

updated diff per review

lld/MachO/UnwindInfoSection.cpp

423–426

(reverted this for now - will send a different patch w.r.t dwarf handling)

This patch will now be mostly NFC

[oontvoo]

rebase

[oontvoo]

use UNWIND_MODE_MASK everywhere

[int3]

The previous form is currently "harmless" and happened to work (because there's no 0x05000000 in the enum set, but as soon as it's defined, it'll stop working)

I guess we can remove this now? Or if you want to be more precise, I think it happens to work because we don't usually get values like 0x04111111 in the input, so there's usually nothing to mask out

lld/MachO/InputFiles.cpp

76–77

don't think this is needed any more

1072–1075

we can rm this now that we're doing it at the top level

[oontvoo]

In D135359#3859014, @int3 wrote:

The previous form is currently "harmless" and happened to work (because there's no 0x05000000 in the enum set, but as soon as it's defined, it'll stop working)

I can see why there might be an issue if 0x11000000 was a defined mode, but I don't think 0x05000000 would be a problem?
<....>
I guess we can remove this now? Or if you want to be more precise, I think it happens to work because we don't usually get values like 0x04111111 in the input, so there's usually nothing to mask out

Sorry, I'd missed your question about the 0x05000000.
Consider the struct: (for x86-64, but same issue can be said for the ARM/64 families):

UNWIND_X86_64_MODE_MASK                              = 0x0F000000,
UNWIND_X86_64_MODE_RBP_FRAME                   = 0x01000000,
UNWIND_X86_64_MODE_STACK_IMMD                = 0x02000000,
UNWIND_X86_64_MODE_STACK_IND                    = 0x03000000,
UNWIND_X86_64_MODE_DWARF                          = 0x04000000,

Previously, we were doing: (encoding & MODE_DWARF) == MODE_DWARF

What I meant was, as soon as a new UNWIND_X86_64_MODE_FOO = 0x05000000 is defined, then the check above would always return true for encoding=MODE_FOO (because (0b0101 & 0b0100) == 0b0100
So no, the explanation still sticks :)

[int3]

d'oh, I get it now, thanks. Could you put that in the commit message? Thanks!

[oontvoo]

updateded commit msg

lld/MachO/InputFiles.cpp

 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/TarWriter.h"
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/TextAPI/Architecture.h"
 #include "llvm/TextAPI/InterfaceFile.h"
 
 #include <type_traits>
 
 using namespace llvm;
 using namespace llvm::MachO;
 using namespace llvm::support::endian;
 using namespace llvm::sys;
 using namespace lld;
 using namespace lld::macho;
 
 // Returns "<internal>", "foo.a(bar.o)", or "baz.o".
ArrayRef<uint8_t> ObjFile::getOptimizationHints() const {
   if (auto *cmd =
           findCommand<linkedit_data_command>(buf, LC_LINKER_OPTIMIZATION_HINT))
     return {buf + cmd->dataoff, cmd->datasize};
   return {};
 }
 
 // Create pointers from symbols to their associated compact unwind entries.
 void ObjFile::registerCompactUnwind(Section &compactUnwindSection) {
   for (const Subsection &subsection : compactUnwindSection.subsections) {
     ConcatInputSection *isec = cast<ConcatInputSection>(subsection.isec);
     // Hack!! Each compact unwind entry (CUE) has its UNSIGNED relocations embed
     // their addends in its data. Thus if ICF operated naively and compared the
     // entire contents of each CUE, entries with identical unwind info but e.g.
     // belonging to different functions would never be considered equivalent. To
     // work around this problem, we remove some parts of the data containing the
     // embedded addends. In particular, we remove the function address and LSDA
     // pointers.  Since these locations are at the start and end of the entry,
     // we can do this using a simple, efficient slice rather than performing a
     // copy.  We are not losing any information here because the embedded
     // addends have already been parsed in the corresponding Reloc structs.
     //
     // Removing these pointers would not be safe if they were pointers to
     // absolute symbols. In that case, there would be no corresponding
     // relocation. However, (AFAIK) MC cannot emit references to absolute
     // symbols for either the function address or the LSDA. However, it *can* do
     // so for the personality pointer, so we are not slicing that field away.
     //
     // Note that we do not adjust the offsets of the corresponding relocations;
     // instead, we rely on `relocateCompactUnwind()` to correctly handle these
     // truncated input sections.
     isec->data = isec->data.slice(target->wordSize, 8 + target->wordSize);
     uint32_t encoding = read32le(isec->data.data() + sizeof(uint32_t));
     // llvm-mc omits CU entries for functions that need DWARF encoding, but
     // `ld -r` doesn't. We can ignore them because we will re-synthesize these
     // CU entries from the DWARF info during the output phase.
-    if ((encoding & target->modeDwarfEncoding) == target->modeDwarfEncoding)
+    if ((encoding & static_cast<uint32_t>(UNWIND_MODE_MASK)) ==
+        target->modeDwarfEncoding)
       continue;
 
     ConcatInputSection *referentIsec;
     for (auto it = isec->relocs.begin(); it != isec->relocs.end();) {
       Reloc &r = *it;
       // CUE::functionAddress is at offset 0. Skip personality & LSDA relocs.
       if (r.offset != 0) {
         ++it;

lld/MachO/Target.h

 #include "llvm/ADT/BitmaskEnum.h"
 #include "llvm/BinaryFormat/MachO.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/MemoryBuffer.h"
 
 #include <cstddef>
 #include <cstdint>
 
+#include "mach-o/compact_unwind_encoding.h"
+
 namespace lld::macho {
 LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();
 
 class Symbol;
 class Defined;
 class DylibSymbol;
 class InputSection;
 class ObjFile;
 
+static_assert(static_cast<uint32_t>(UNWIND_X86_64_MODE_MASK) ==
+                  static_cast<uint32_t>(UNWIND_X86_MODE_MASK) &&
+              static_cast<uint32_t>(UNWIND_ARM64_MODE_MASK) ==
+                  static_cast<uint32_t>(UNWIND_X86_64_MODE_MASK));
+
+// Since the mode masks have the same value on all targets, define
+// a common one for convenience.
+constexpr uint32_t UNWIND_MODE_MASK = UNWIND_X86_64_MODE_MASK;
+
 class TargetInfo {
 public:
   template <class LP> TargetInfo(LP) {
     // Having these values available in TargetInfo allows us to access them
     // without having to resort to templates.
     magic = LP::magic;
     pageZeroSize = LP::pageZeroSize;
     headerSize = sizeof(typename LP::mach_header);

lld/MachO/UnwindInfoSection.cpp

static bool canFoldEncoding(compact_unwind_encoding_t encoding) {
   //  large to encode in the compact unwind encoding.  Instead it requires that
   //  the function contains "subq $nnnnnnnn,RSP" in its prolog.  The compact
   //  encoding contains the offset to the nnnnnnnn value in the function in
   //  UNWIND_X86_64_FRAMELESS_STACK_SIZE.
   // Since this means the unwinder has to look at the `subq` in the function
   // of the unwind info's unwind address, two functions that have identical
   // unwind info can't be folded if it's using this encoding since both
   // entries need unique addresses.
-  static_assert(static_cast<uint32_t>(UNWIND_X86_64_MODE_MASK) ==
-                static_cast<uint32_t>(UNWIND_X86_MODE_MASK));
   static_assert(static_cast<uint32_t>(UNWIND_X86_64_MODE_STACK_IND) ==
                 static_cast<uint32_t>(UNWIND_X86_MODE_STACK_IND));
   if ((target->cpuType == CPU_TYPE_X86_64 || target->cpuType == CPU_TYPE_X86) &&
-      (encoding & UNWIND_X86_64_MODE_MASK) == UNWIND_X86_64_MODE_STACK_IND) {
+      (encoding & UNWIND_MODE_MASK) == UNWIND_X86_64_MODE_STACK_IND) {
     // FIXME: Consider passing in the two function addresses and getting
     // their two stack sizes off the `subq` and only returning false if they're
     // actually different.
     return false;
   }
   return true;
 }
 
 // Scan the __LD,__compact_unwind entries and compute the space needs of
 // __TEXT,__unwind_info and __TEXT,__eh_frame.