[NFCI][mlir][Tests] Rename identifiers minor/major to avoid clashes with system headers

[rogfer01]

Summary

Identifiers major and minor are often already taken in POSIX systems due to their presence in <sys/types.h> as part of the makedev library function.

• Linux: https://man7.org/linux/man-pages/man3/makedev.3.html
• FreeBSD: https://man.freebsd.org/cgi/man.cgi?query=makedev

Recent versions of glibc have moved this definitions from <sys/types.h> to sys/sysmacros.h, so newer systems may not be impacted. From the Linux link above:

Depending on the version, glibc also exposes definitions for these macros from <sys/types.h> if suitable feature test macros are defined. However, this behavior was deprecated in glibc 2.25, and since glibc 2.28, <sys/types.h> no longer provides these definitions.

In particular Ubuntu 18.04 LTS (albeit a bit old now) still provides them in <sys/types.h> along with a loud warning. Ubuntu 22.04 LTS does not have this problem. I have not been able to test it on 20.04 LTS.

Other systems, such as FreeBSD still use <sys/types.h> so there is not much we can do I think. I have verified that the issue happens on a VM running FreeBSD 13.2.

I initially considered doing:

#if defined(major)
#undef major
#endif
#if defined(minor)
#undef minor
#endif

but on a second thought I preferred the longer route of avoiding those identifiers entirely.

[mfrancio]

mlir/test/lib/Dialect/Test/TestDialect.h

68–74

nit: camel case

70

Can we add a comment explaining the name clash here?

[rogfer01]

ChangeLog:

• Address review comments

mlir/test/lib/Dialect/Test/TestDialect.h

 namespace test {
 
 //===----------------------------------------------------------------------===//
 // TestDialect version utilities
 //===----------------------------------------------------------------------===//
 
 struct TestDialectVersion : public mlir::DialectVersion {
   TestDialectVersion() = default;
-  TestDialectVersion(uint32_t _major, uint32_t _minor)
-      : major(_major), minor(_minor){};
-  uint32_t major = 2;
-  uint32_t minor = 0;
+  TestDialectVersion(uint32_t majorVersion, uint32_t minorVersion)
+      : major_(majorVersion), minor_(minorVersion){};
+  // We cannot use 'major' and 'minor' here because these identifiers may
+  // already be used by <sys/types.h> on many POSIX systems including Linux and
+  // FreeBSD.
+  uint32_t major_ = 2;
+  uint32_t minor_ = 0;
 };
 
 // Define some classes to exercises the Properties feature.
 
 struct PropertiesWithCustomPrint {
   /// A shared_ptr to a const object is safe: it is equivalent to a value-based
   /// member. Here the label will be deallocated when the last operation
   /// refering to it is destroyed. However there is no pool-allocation: this is

mlir/test/lib/Dialect/Test/TestDialect.cpp

TestVersionedOpA::readProperties(::mlir::DialectBytecodeReader &reader,
   // Check if we have a version. If not, assume we are parsing the current
   // version.
   auto maybeVersion = reader.getDialectVersion("test");
   if (succeeded(maybeVersion)) {
     // If version is less than 2.0, there is no additional attribute to parse.
     // We can materialize missing properties post parsing before verification.
     const auto *version =
         reinterpret_cast<const TestDialectVersion *>(*maybeVersion);
-    if ((version->major < 2)) {
+    if ((version->major_ < 2)) {
       return success();
     }
   }
 
   if (::mlir::failed(reader.readAttribute(prop.modifier)))
     return ::mlir::failure();
   return ::mlir::success();
 }
::mlir::LogicalResult TestOpWithVersionedProperties::readFromMlirBytecode(
   // version.
   auto maybeVersion = reader.getDialectVersion("test");
   bool needToParseAnotherInt = true;
   if (succeeded(maybeVersion)) {
     // If version is less than 2.0, there is no additional attribute to parse.
     // We can materialize missing properties post parsing before verification.
     const auto *version =
         reinterpret_cast<const TestDialectVersion *>(*maybeVersion);
-    if ((version->major < 2))
+    if ((version->major_ < 2))
       needToParseAnotherInt = false;
   }
   if (needToParseAnotherInt && failed(reader.readVarInt(value2)))
     return failure();
 
   prop.value1 = value1;
   prop.value2 = value2;
   return success();

mlir/test/lib/Dialect/Test/TestDialectInterfaces.cpp

struct TestBytecodeDialectInterface : public BytecodeDialectInterface {
 
   Attribute readAttribute(DialectBytecodeReader &reader) const final {
     auto versionOr = reader.getDialectVersion("test");
     // Assume current version if not available through the reader.
     const auto version =
         (succeeded(versionOr))
             ? *reinterpret_cast<const TestDialectVersion *>(*versionOr)
             : TestDialectVersion();
-    if (version.major < 2)
+    if (version.major_ < 2)
       return readAttrOldEncoding(reader);
-    if (version.major == 2 && version.minor == 0)
+    if (version.major_ == 2 && version.minor_ == 0)
       return readAttrNewEncoding(reader);
     // Forbid reading future versions by returning nullptr.
     return Attribute();
   }
 
   // Emit a specific version of the dialect.
   void writeVersion(DialectBytecodeWriter &writer) const final {
     auto version = TestDialectVersion();
-    writer.writeVarInt(version.major); // major
-    writer.writeVarInt(version.minor); // minor
+    writer.writeVarInt(version.major_); // major
+    writer.writeVarInt(version.minor_); // minor
   }
 
   std::unique_ptr<DialectVersion>
   readVersion(DialectBytecodeReader &reader) const final {
-    uint64_t major, minor;
-    if (failed(reader.readVarInt(major)) || failed(reader.readVarInt(minor)))
+    uint64_t major_, minor_;
+    if (failed(reader.readVarInt(major_)) || failed(reader.readVarInt(minor_)))
       return nullptr;
     auto version = std::make_unique<TestDialectVersion>();
-    version->major = major;
-    version->minor = minor;
+    version->major_ = major_;
+    version->minor_ = minor_;
     return version;
   }
 
   LogicalResult upgradeFromVersion(Operation *topLevelOp,
                                    const DialectVersion &version_) const final {
     const auto &version = static_cast<const TestDialectVersion &>(version_);
-    if ((version.major == 2) && (version.minor == 0))
+    if ((version.major_ == 2) && (version.minor_ == 0))
       return success();
-    if (version.major > 2 || (version.major == 2 && version.minor > 0)) {
+    if (version.major_ > 2 || (version.major_ == 2 && version.minor_ > 0)) {
       return topLevelOp->emitError()
              << "current test dialect version is 2.0, can't parse version: "
-             << version.major << "." << version.minor;
+             << version.major_ << "." << version.minor_;
     }
     // Prior version 2.0, the old op supported only a single attribute called
     // "dimensions". We can perform the upgrade.
     topLevelOp->walk([](TestVersionedOpA op) {
       // Prior version 2.0, `readProperties` did not process the modifier
       // attribute. Handle that according to the version here.
       auto &prop = op.getProperties();
       prop.modifier = BoolAttr::get(op->getContext(), false);

mlir/test/lib/IR/TestBytecodeCallbacks.cpp

 namespace {
 class TestDialectVersionParser : public cl::parser<test::TestDialectVersion> {
 public:
   TestDialectVersionParser(cl::Option &O)
       : cl::parser<test::TestDialectVersion>(O) {}
 
   bool parse(cl::Option &O, StringRef /*argName*/, StringRef arg,
              test::TestDialectVersion &v) {
-    long long major, minor;
-    if (getAsSignedInteger(arg.split(".").first, 10, major))
+    long long major_, minor_;
+    if (getAsSignedInteger(arg.split(".").first, 10, major_))
       return O.error("Invalid argument '" + arg);
-    if (getAsSignedInteger(arg.split(".").second, 10, minor))
+    if (getAsSignedInteger(arg.split(".").second, 10, minor_))
       return O.error("Invalid argument '" + arg);
-    v = test::TestDialectVersion(major, minor);
+    v = test::TestDialectVersion(major_, minor_);
     // Returns true on error.
     return false;
   }
   static void print(raw_ostream &os, const test::TestDialectVersion &v) {
-    os << v.major << "." << v.minor;
+    os << v.major_ << "." << v.minor_;
   };
 };
 
 /// This is a test pass which uses callbacks to encode attributes and types in a
 /// custom fashion.
 struct TestBytecodeCallbackPass
     : public PassWrapper<TestBytecodeCallbackPass, OperationPass<ModuleOp>> {
   MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(TestBytecodeCallbackPass)
private:
   void runTest0(Operation *op) {
     auto newCtx = std::make_shared<MLIRContext>();
     test::TestDialectVersion targetEmissionVersion = targetVersion;
     BytecodeWriterConfig writeConfig;
     writeConfig.attachTypeCallback(
         [&](Type entryValue, std::optional<StringRef> &dialectGroupName,
             DialectBytecodeWriter &writer) -> LogicalResult {
           // Do not override anything if version less than 2.0.
-          if (targetEmissionVersion.major >= 2)
+          if (targetEmissionVersion.major_ >= 2)
             return failure();
 
           // For version less than 2.0, override the encoding of IntegerType.
           if (auto type = llvm::dyn_cast<IntegerType>(entryValue)) {
             llvm::outs() << "Overriding IntegerType encoding...\n";
             dialectGroupName = StringLiteral("funky");
             writer.writeVarInt(/* IntegerType */ 999);
             writer.writeVarInt(type.getWidth() << 2 | type.getSignedness());
parseConfig.getBytecodeReaderConfig().attachTypeCallback(
               reinterpret_cast<const test::TestDialectVersion *>(*versionOr);
 
           // TODO: once back-deployment is formally supported,
           // `targetEmissionVersion` will be encoded in the bytecode file, and
           // exposed through the versionMap. Right now though this is not yet
           // supported. For the purpose of the test, just use
           // `targetEmissionVersion`.
           (void)version;
-          if (targetEmissionVersion.major >= 2)
+          if (targetEmissionVersion.major_ >= 2)
             return success();
 
           // `dialectName` is the name of the group we have the opportunity to
           // override. In this case, override only the dialect group "funky",
           // for which does not exist in memory.
           if (dialectName != StringLiteral("funky"))
             return success();