Reapply "[MC] Use a variant to hold MCCFIInstruction state (NFC)"

[slinder1]

This reverts commit 8217c64.

I asked Claude Sonnet 4.5 to hazard a guess at what was causing the buildbot failure, and it seems to have correctly pinned it on a bug in GCC7, although it didn't give a working fix.

I worked out of a gcc:7.4 docker container and moved code around until MCDwarf.cpp.o compiled without errors or warnings, so hopefully that's sufficient to fix the bot.

Is there an easy way to do a build before landing to confirm, @Kewen12 / @ronlieb ?

[llvmbot]

@llvm/pr-subscribers-llvm-mc

Author: Scott Linder (slinder1)

Changes

This reverts commit 8217c64.

I asked Claude Sonnet 4.5 to hazard a guess at what was causing the buildbot failure, and it seems to have correctly pinned it on a bug in GCC7, although it didn't give a working fix.

I worked out of a gcc:7.4 docker container and moved code around until MCDwarf.cpp.o compiled without errors or warnings, so hopefully that's sufficient to fix the bot.

Is there an easy way to do a build before landing to confirm, @Kewen12 / @ronlieb ?

---

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

1 Files Affected:

• (modified) llvm/include/llvm/MC/MCDwarf.h (+71-77)

diff --git a/llvm/include/llvm/MC/MCDwarf.h b/llvm/include/llvm/MC/MCDwarf.h
index 9944a9a92ab1f..ddfac483c73e7 100644
--- a/llvm/include/llvm/MC/MCDwarf.h
+++ b/llvm/include/llvm/MC/MCDwarf.h
@@ -29,6 +29,7 @@
 #include <optional>
 #include <string>
 #include <utility>
+#include <variant>
 #include <vector>
 
 namespace llvm {
@@ -531,67 +532,58 @@ class MCCFIInstruction {
     OpValOffset,
   };
 
+  // Held in ExtraFields for most common OpTypes, exceptions follow.
+  struct CommonFields {
+    unsigned Register;
+    int64_t Offset;
+    unsigned Register2;
+    unsigned AddressSpace;
+    // FIXME: Workaround for GCC7 bug with nested class used as std::variant
+    // alternative where the compiler really wants a user-defined default
+    // constructor. Once we no longer support GCC7 these constructors can be
+    // replaced with default member initializers and aggregate initialization.
+    CommonFields(unsigned Reg,
+                 int64_t Off = 0,
+                 unsigned Reg2 = std::numeric_limits<unsigned>::max(),
+                 unsigned AddrSpace = 0)
+        : Register(Reg), Offset(Off), Register2(Reg2), AddressSpace(AddrSpace) {
+    }
+    CommonFields() : CommonFields(std::numeric_limits<unsigned>::max()) {}
+  };
+  // Held in ExtraFields when OpEscape.
+  struct EscapeFields {
+    std::vector<char> Values;
+    std::string Comment;
+  };
+  // Held in ExtraFields when OpLabel.
+  struct LabelFields {
+    MCSymbol *CfiLabel = nullptr;
+  };
+
 private:
   MCSymbol *Label;
-  union {
-    struct {
-      unsigned Register;
-      int64_t Offset;
-    } RI;
-    struct {
-      unsigned Register;
-      int64_t Offset;
-      unsigned AddressSpace;
-    } RIA;
-    struct {
-      unsigned Register;
-      unsigned Register2;
-    } RR;
-    MCSymbol *CfiLabel;
-  } U;
+  std::variant<CommonFields, EscapeFields, LabelFields> ExtraFields;
   OpType Operation;
   SMLoc Loc;
-  std::vector<char> Values;
-  std::string Comment;
 
-  MCCFIInstruction(OpType Op, MCSymbol *L, unsigned R, int64_t O, SMLoc Loc,
-                   StringRef V = "", StringRef Comment = "")
-      : Label(L), Operation(Op), Loc(Loc), Values(V.begin(), V.end()),
-        Comment(Comment) {
-    assert(Op != OpRegister && Op != OpLLVMDefAspaceCfa);
-    U.RI = {R, O};
-  }
-  MCCFIInstruction(OpType Op, MCSymbol *L, unsigned R1, unsigned R2, SMLoc Loc)
-      : Label(L), Operation(Op), Loc(Loc) {
-    assert(Op == OpRegister);
-    U.RR = {R1, R2};
-  }
-  MCCFIInstruction(OpType Op, MCSymbol *L, unsigned R, int64_t O, unsigned AS,
-                   SMLoc Loc)
-      : Label(L), Operation(Op), Loc(Loc) {
-    assert(Op == OpLLVMDefAspaceCfa);
-    U.RIA = {R, O, AS};
-  }
-
-  MCCFIInstruction(OpType Op, MCSymbol *L, MCSymbol *CfiLabel, SMLoc Loc)
-      : Label(L), Operation(Op), Loc(Loc) {
-    assert(Op == OpLabel);
-    U.CfiLabel = CfiLabel;
-  }
+  template <class FieldsType>
+  MCCFIInstruction(OpType Op, MCSymbol *L, FieldsType &&EF, SMLoc Loc)
+      : Label(L), ExtraFields(std::forward<FieldsType>(EF)), Operation(Op),
+        Loc(Loc) {}
 
 public:
   /// .cfi_def_cfa defines a rule for computing CFA as: take address from
   /// Register and add Offset to it.
   static MCCFIInstruction cfiDefCfa(MCSymbol *L, unsigned Register,
                                     int64_t Offset, SMLoc Loc = {}) {
-    return MCCFIInstruction(OpDefCfa, L, Register, Offset, Loc);
+    return {OpDefCfa, L, CommonFields{Register, Offset}, Loc};
   }
 
   /// .cfi_def_cfa_register modifies a rule for computing CFA. From now
   /// on Register will be used instead of the old one. Offset remains the same.
   static MCCFIInstruction createDefCfaRegister(MCSymbol *L, unsigned Register,
                                                SMLoc Loc = {}) {
-    return MCCFIInstruction(OpDefCfaRegister, L, Register, INT64_C(0), Loc);
+    return {OpDefCfaRegister, L, CommonFields{Register}, Loc};
   }
 
   /// .cfi_def_cfa_offset modifies a rule for computing CFA. Register
@@ -599,7 +591,7 @@ class MCCFIInstruction {
   /// that will be added to a defined register to the compute CFA address.
   static MCCFIInstruction cfiDefCfaOffset(MCSymbol *L, int64_t Offset,
                                           SMLoc Loc = {}) {
-    return MCCFIInstruction(OpDefCfaOffset, L, 0, Offset, Loc);
+    return {OpDefCfaOffset, L, CommonFields{0, Offset}, Loc};
   }
 
   /// .cfi_adjust_cfa_offset Same as .cfi_def_cfa_offset, but
@@ -607,7 +599,7 @@ class MCCFIInstruction {
   /// offset.
   static MCCFIInstruction createAdjustCfaOffset(MCSymbol *L, int64_t Adjustment,
                                                 SMLoc Loc = {}) {
-    return MCCFIInstruction(OpAdjustCfaOffset, L, 0, Adjustment, Loc);
+    return {OpAdjustCfaOffset, L, CommonFields{0, Adjustment}, Loc};
   }
 
   // FIXME: Update the remaining docs to use the new proposal wording.
@@ -618,15 +610,15 @@ class MCCFIInstruction {
                                                  int64_t Offset,
                                                  unsigned AddressSpace,
                                                  SMLoc Loc) {
-    return MCCFIInstruction(OpLLVMDefAspaceCfa, L, Register, Offset,
-                            AddressSpace, Loc);
+    return {OpLLVMDefAspaceCfa, L,
+            CommonFields{Register, Offset, 0, AddressSpace}, Loc};
   }
 
   /// .cfi_offset Previous value of Register is saved at offset Offset
   /// from CFA.
   static MCCFIInstruction createOffset(MCSymbol *L, unsigned Register,
                                        int64_t Offset, SMLoc Loc = {}) {
-    return MCCFIInstruction(OpOffset, L, Register, Offset, Loc);
+    return {OpOffset, L, CommonFields{Register, Offset}, Loc};
   }
 
   /// .cfi_rel_offset Previous value of Register is saved at offset
@@ -634,30 +626,30 @@ class MCCFIInstruction {
   /// using the known displacement of the CFA register from the CFA.
   static MCCFIInstruction createRelOffset(MCSymbol *L, unsigned Register,
                                           int64_t Offset, SMLoc Loc = {}) {
-    return MCCFIInstruction(OpRelOffset, L, Register, Offset, Loc);
+    return {OpRelOffset, L, CommonFields{Register, Offset}, Loc};
   }
 
   /// .cfi_register Previous value of Register1 is saved in
   /// register Register2.
   static MCCFIInstruction createRegister(MCSymbol *L, unsigned Register1,
                                          unsigned Register2, SMLoc Loc = {}) {
-    return MCCFIInstruction(OpRegister, L, Register1, Register2, Loc);
+    return {OpRegister, L, CommonFields{Register1, 0, Register2}, Loc};
   }
 
   /// .cfi_window_save SPARC register window is saved.
   static MCCFIInstruction createWindowSave(MCSymbol *L, SMLoc Loc = {}) {
-    return MCCFIInstruction(OpWindowSave, L, 0, INT64_C(0), Loc);
+    return {OpWindowSave, L, CommonFields{}, Loc};
   }
 
   /// .cfi_negate_ra_state AArch64 negate RA state.
   static MCCFIInstruction createNegateRAState(MCSymbol *L, SMLoc Loc = {}) {
-    return MCCFIInstruction(OpNegateRAState, L, 0, INT64_C(0), Loc);
+    return {OpNegateRAState, L, CommonFields{}, Loc};
   }
 
   /// .cfi_negate_ra_state_with_pc AArch64 negate RA state with PC.
   static MCCFIInstruction createNegateRAStateWithPC(MCSymbol *L,
                                                     SMLoc Loc = {}) {
-    return MCCFIInstruction(OpNegateRAStateWithPC, L, 0, INT64_C(0), Loc);
+    return {OpNegateRAStateWithPC, L, CommonFields{}, Loc};
   }
 
   /// .cfi_restore says that the rule for Register is now the same as it
@@ -665,104 +657,106 @@ class MCCFIInstruction {
   /// by .cfi_startproc were executed.
   static MCCFIInstruction createRestore(MCSymbol *L, unsigned Register,
                                         SMLoc Loc = {}) {
-    return MCCFIInstruction(OpRestore, L, Register, INT64_C(0), Loc);
+    return {OpRestore, L, CommonFields{Register}, Loc};
   }
 
   /// .cfi_undefined From now on the previous value of Register can't be
   /// restored anymore.
   static MCCFIInstruction createUndefined(MCSymbol *L, unsigned Register,
                                           SMLoc Loc = {}) {
-    return MCCFIInstruction(OpUndefined, L, Register, INT64_C(0), Loc);
+    return {OpUndefined, L, CommonFields{Register}, Loc};
   }
 
   /// .cfi_same_value Current value of Register is the same as in the
   /// previous frame. I.e., no restoration is needed.
   static MCCFIInstruction createSameValue(MCSymbol *L, unsigned Register,
                                           SMLoc Loc = {}) {
-    return MCCFIInstruction(OpSameValue, L, Register, INT64_C(0), Loc);
+    return {OpSameValue, L, CommonFields{Register}, Loc};
   }
 
   /// .cfi_remember_state Save all current rules for all registers.
   static MCCFIInstruction createRememberState(MCSymbol *L, SMLoc Loc = {}) {
-    return MCCFIInstruction(OpRememberState, L, 0, INT64_C(0), Loc);
+    return {OpRememberState, L, CommonFields{}, Loc};
   }
 
   /// .cfi_restore_state Restore the previously saved state.
   static MCCFIInstruction createRestoreState(MCSymbol *L, SMLoc Loc = {}) {
-    return MCCFIInstruction(OpRestoreState, L, 0, INT64_C(0), Loc);
+    return {OpRestoreState, L, CommonFields{}, Loc};
   }
 
   /// .cfi_escape Allows the user to add arbitrary bytes to the unwind
   /// info.
   static MCCFIInstruction createEscape(MCSymbol *L, StringRef Vals,
                                        SMLoc Loc = {}, StringRef Comment = "") {
-    return MCCFIInstruction(OpEscape, L, 0, 0, Loc, Vals, Comment);
+    return {OpEscape, L,
+            EscapeFields{std::vector<char>(Vals.begin(), Vals.end()),
+                         Comment.str()},
+            Loc};
   }
 
   /// A special wrapper for .cfi_escape that indicates GNU_ARGS_SIZE
   static MCCFIInstruction createGnuArgsSize(MCSymbol *L, int64_t Size,
                                             SMLoc Loc = {}) {
-    return MCCFIInstruction(OpGnuArgsSize, L, 0, Size, Loc);
+    return {OpGnuArgsSize, L, CommonFields{0, Size}, Loc};
   }
 
   static MCCFIInstruction createLabel(MCSymbol *L, MCSymbol *CfiLabel,
                                       SMLoc Loc) {
-    return MCCFIInstruction(OpLabel, L, CfiLabel, Loc);
+    return {OpLabel, L, LabelFields{CfiLabel}, Loc};
   }
 
   /// .cfi_val_offset Previous value of Register is offset Offset from the
   /// current CFA register.
   static MCCFIInstruction createValOffset(MCSymbol *L, unsigned Register,
                                           int64_t Offset, SMLoc Loc = {}) {
-    return MCCFIInstruction(OpValOffset, L, Register, Offset, Loc);
+    return {OpValOffset, L, CommonFields{Register, Offset}, Loc};
   }
 
   OpType getOperation() const { return Operation; }
   MCSymbol *getLabel() const { return Label; }
 
   unsigned getRegister() const {
-    if (Operation == OpRegister)
-      return U.RR.Register;
-    if (Operation == OpLLVMDefAspaceCfa)
-      return U.RIA.Register;
     assert(Operation == OpDefCfa || Operation == OpOffset ||
            Operation == OpRestore || Operation == OpUndefined ||
            Operation == OpSameValue || Operation == OpDefCfaRegister ||
-           Operation == OpRelOffset || Operation == OpValOffset);
-    return U.RI.Register;
+           Operation == OpRelOffset || Operation == OpValOffset ||
+           Operation == OpRegister || Operation == OpLLVMDefAspaceCfa);
+    return std::get<CommonFields>(ExtraFields).Register;
   }
 
   unsigned getRegister2() const {
     assert(Operation == OpRegister);
-    return U.RR.Register2;
+    return std::get<CommonFields>(ExtraFields).Register2;
   }
 
   unsigned getAddressSpace() const {
     assert(Operation == OpLLVMDefAspaceCfa);
-    return U.RIA.AddressSpace;
+    return std::get<CommonFields>(ExtraFields).AddressSpace;
   }
 
   int64_t getOffset() const {
-    if (Operation == OpLLVMDefAspaceCfa)
-      return U.RIA.Offset;
     assert(Operation == OpDefCfa || Operation == OpOffset ||
            Operation == OpRelOffset || Operation == OpDefCfaOffset ||
            Operation == OpAdjustCfaOffset || Operation == OpGnuArgsSize ||
-           Operation == OpValOffset);
-    return U.RI.Offset;
+           Operation == OpValOffset || Operation == OpLLVMDefAspaceCfa);
+    return std::get<CommonFields>(ExtraFields).Offset;
   }
 
   MCSymbol *getCfiLabel() const {
     assert(Operation == OpLabel);
-    return U.CfiLabel;
+    return std::get<LabelFields>(ExtraFields).CfiLabel;
   }
 
   StringRef getValues() const {
     assert(Operation == OpEscape);
+    auto &Values = std::get<EscapeFields>(ExtraFields).Values;
     return StringRef(&Values[0], Values.size());
   }
 
-  StringRef getComment() const { return Comment; }
+  StringRef getComment() const {
+    assert(Operation == OpEscape);
+    return std::get<EscapeFields>(ExtraFields).Comment;
+  }
   SMLoc getLoc() const { return Loc; }
 };
 

[github-actions]

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

[Kewen12]

Thanks for fixing it! The failed bot (https://lab.llvm.org/buildbot/#/builders/140/builds/34645) is running SUSE15 with an old version of gcc 7.5.0. It should work if gcc 7.4 compiled.

Please feel free to land to unblock your progress. Let's monitor the build (fix/revert if necessary).