llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: None (royitaqi) <details> <summary>Changes</summary> **Context**: See previous attempts: #<!-- -->142704, #<!-- -->143633 **Problem**: When `ObjectFileMachO` parses a Mach-O file (including yaml data in lldb unit tests) which doesn't have load commands like `LC_BUILD_VERSION` or `LC_VERSION_MIN_*`, its object format is mistakenly reported as `ELF`. Had the load commands existed, they would have caused `ObjectFileMachO` to set the correct OS name into the `Triple` ([here](https://github.com/llvm/llvm-project/blob/52a6492136ef43462c68efa88a0276bb66ee8c52/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp#L5158-L5174) and [here](https://github.com/llvm/llvm-project/blob/52a6492136ef43462c68efa88a0276bb66ee8c52/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp#L5109-L5125)), which would have led to a correct default `MachO` object format in `getDefaultFormat()` ([here](https://github.com/llvm/llvm-project/blob/e37707b1e85cfc07fe75fd6b7e5d41963c52a8ec/llvm/lib/TargetParser/Triple.cpp#L937)). **Alternative approaches considered**: 1. Previously, #<!-- -->142704 tries to fix this by setting the correct object format in `ObjectFileMachO::GetAllArchSpecs()` for all input files. However, such wide range of explicit assignment (even when the files have the load commands) led the `Triple` to report "-macho" in its string form, causing change in UI and a test failure which depends on the UI. 2. #<!-- -->143633 updates `getDefaultFormat()`, so that it will return `MachO` when the OS is not set and the vendor is `Apple`. However, it's a change to one of the foundational llvm classes - it's not clear if such impact is worth it. 3. Another approach is to reject such Mach-O files outright. However, there is no a Mach-O spec which states that the load commands are required, plus it is convenient to be able to leave them out when writing tests. **This patch** tries to strike a good balance of the above: 1. Set the correct object format in `ObjectFileMachO::GetAllArchSpecs()`, but only when the load commands do not exist. Compared to #<!-- -->142704, this patch minimizes the cases where the `Triple` string will change. 2. Fix any existing tests that are broken by this change (should be minimal), by adding the load commands. 3. This patch shouldn't change anything for files/tests which already have the load commands. --- Full diff: https://github.com/llvm/llvm-project/pull/144177.diff 2 Files Affected: - (modified) lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp (+4) - (modified) lldb/unittests/ObjectFile/MachO/TestObjectFileMachO.cpp (+124) ``````````diff diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp index 3950454b7c90e..98bd3ea9dcfe9 100644 --- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp +++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp @@ -5255,6 +5255,10 @@ void ObjectFileMachO::GetAllArchSpecs(const llvm::MachO::mach_header &header, } if (!found_any) { + // Explicitly set the object format to Mach-O if no LC_BUILD_VERSION or + // LC_VERSION_MIN_* are found. Without this, the object format will default + // to ELF (see `getDefaultFormat()` in `Triple.cpp`), which is wrong. + base_triple.setObjectFormat(llvm::Triple::MachO); add_triple(base_triple); } } diff --git a/lldb/unittests/ObjectFile/MachO/TestObjectFileMachO.cpp b/lldb/unittests/ObjectFile/MachO/TestObjectFileMachO.cpp index 0ef2d0b85fd36..dc1bfa86b0abf 100644 --- a/lldb/unittests/ObjectFile/MachO/TestObjectFileMachO.cpp +++ b/lldb/unittests/ObjectFile/MachO/TestObjectFileMachO.cpp @@ -94,4 +94,128 @@ TEST_F(ObjectFileMachOTest, IndirectSymbolsInTheSharedCache) { for (size_t i = 0; i < 10; i++) OF->ParseSymtab(symtab); } + +TEST_F(ObjectFileMachOTest, ObjectFormatWithVersionLoadCommand) { + // A Mach-O file of arm64 CPU type and macOS 10.14.0 + const char *yamldata = R"( +--- !mach-o +FileHeader: + magic: 0xFEEDFACF + cputype: 0x0100000C + cpusubtype: 0x00000000 + filetype: 0x00000001 + ncmds: 1 + sizeofcmds: 176 + flags: 0x00002000 + reserved: 0x00000000 +LoadCommands: + - cmd: LC_BUILD_VERSION + cmdsize: 24 + platform: 1 + minos: 658944 + sdk: 658944 + ntools: 0 + - cmd: LC_SEGMENT_64 + cmdsize: 152 + segname: __TEXT + vmaddr: 0 + vmsize: 4 + fileoff: 208 + filesize: 4 + maxprot: 7 + initprot: 7 + nsects: 1 + flags: 0 + Sections: + - sectname: __text + segname: __TEXT + addr: 0x0000000000000000 + content: 'AABBCCDD' + size: 4 + offset: 208 + align: 0 + reloff: 0x00000000 + nreloc: 0 + flags: 0x80000400 + reserved1: 0x00000000 + reserved2: 0x00000000 + reserved3: 0x00000000 +... +)"; + + // Perform setup. + llvm::Expected<TestFile> file = TestFile::fromYaml(yamldata); + EXPECT_THAT_EXPECTED(file, llvm::Succeeded()); + auto module_sp = std::make_shared<Module>(file->moduleSpec()); + ASSERT_NE(module_sp, nullptr); + auto object_file = module_sp->GetObjectFile(); + ASSERT_NE(object_file, nullptr); + + // Verify that the object file is recognized as Mach-O. + ASSERT_EQ(object_file->GetArchitecture().GetTriple().getObjectFormat(), + llvm::Triple::MachO); + + // Verify that the triple string does NOT contain "-macho". + ASSERT_EQ(object_file->GetArchitecture().GetTriple().str(), + "arm64-apple-macosx10.14.0"); +} + +TEST_F(ObjectFileMachOTest, ObjectFormatWithoutVersionLoadCommand) { + // A Mach-O file of arm64 CPU type, without load command LC_BUILD_VERSION. + const char *yamldata = R"( +--- !mach-o +FileHeader: + magic: 0xFEEDFACF + cputype: 0x0100000C + cpusubtype: 0x00000000 + filetype: 0x00000001 + ncmds: 1 + sizeofcmds: 152 + flags: 0x00002000 + reserved: 0x00000000 +LoadCommands: + - cmd: LC_SEGMENT_64 + cmdsize: 152 + segname: __TEXT + vmaddr: 0 + vmsize: 4 + fileoff: 184 + filesize: 4 + maxprot: 7 + initprot: 7 + nsects: 1 + flags: 0 + Sections: + - sectname: __text + segname: __TEXT + addr: 0x0000000000000000 + content: 'AABBCCDD' + size: 4 + offset: 184 + align: 0 + reloff: 0x00000000 + nreloc: 0 + flags: 0x80000400 + reserved1: 0x00000000 + reserved2: 0x00000000 + reserved3: 0x00000000 +... +)"; + + // Perform setup. + llvm::Expected<TestFile> file = TestFile::fromYaml(yamldata); + EXPECT_THAT_EXPECTED(file, llvm::Succeeded()); + auto module_sp = std::make_shared<Module>(file->moduleSpec()); + ASSERT_NE(module_sp, nullptr); + auto object_file = module_sp->GetObjectFile(); + ASSERT_NE(object_file, nullptr); + + // Verify that the object file is recognized as Mach-O. + ASSERT_EQ(object_file->GetArchitecture().GetTriple().getObjectFormat(), + llvm::Triple::MachO); + + // Verify that the triple string contains "-macho". + ASSERT_EQ(object_file->GetArchitecture().GetTriple().str(), + "arm64-apple--macho"); +} #endif `````````` </details> https://github.com/llvm/llvm-project/pull/144177 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
