Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BinaryFormat] Adjust OSABI functions and add unittests #90270

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Apr 26, 2024

Adjust #89280:

  • ELFOSABI_LINUX is a historical alias that should not be used in new
    code. readelf -h displays "UNIX - GNU" instead of "Linux".
  • "OS" is inappropriate. Some values are architecture-specific, e.g.
    ELFOSABI_ARM.
  • Drop lowercase, which seems a job of the caller.

Add some unittests.

Created using spr 1.3.5-bogner
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 26, 2024

@llvm/pr-subscribers-llvm-binary-utilities

Author: Fangrui Song (MaskRay)

Changes

Adjust #89280:
ELFOSABI_LINUX is a historical alias that should not be used in new
code. readelf -h displays "UNIX - GNU" instead of "Linux". Add some
unittests.


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

3 Files Affected:

  • (modified) llvm/lib/BinaryFormat/ELF.cpp (+3-3)
  • (modified) llvm/unittests/BinaryFormat/CMakeLists.txt (+1)
  • (added) llvm/unittests/BinaryFormat/ELFTest.cpp (+32)
diff --git a/llvm/lib/BinaryFormat/ELF.cpp b/llvm/lib/BinaryFormat/ELF.cpp
index 8c10ed1a980bbc..a07e5423a2ab66 100644
--- a/llvm/lib/BinaryFormat/ELF.cpp
+++ b/llvm/lib/BinaryFormat/ELF.cpp
@@ -573,7 +573,7 @@ uint8_t ELF::convertOSToOSAbi(StringRef OS) {
   return StringSwitch<uint16_t>(LowerOS)
       .StartsWith("hpux", ELFOSABI_HPUX)
       .StartsWith("netbsd", ELFOSABI_NETBSD)
-      .StartsWith("linux", ELFOSABI_LINUX)
+      .StartsWith("gnu", ELFOSABI_GNU)
       .StartsWith("hurd", ELFOSABI_HURD)
       .StartsWith("solaris", ELFOSABI_SOLARIS)
       .StartsWith("aix", ELFOSABI_AIX)
@@ -603,8 +603,8 @@ StringRef ELF::convertOSAbiToOS(uint8_t OSAbi) {
     return "hpux";
   case ELFOSABI_NETBSD:
     return "netbsd";
-  case ELFOSABI_LINUX:
-    return "linux";
+  case ELFOSABI_GNU:
+    return "gnu";
   case ELFOSABI_HURD:
     return "hurd";
   case ELFOSABI_SOLARIS:
diff --git a/llvm/unittests/BinaryFormat/CMakeLists.txt b/llvm/unittests/BinaryFormat/CMakeLists.txt
index f0c42a0dd02b8e..40d3bc4dca0b66 100644
--- a/llvm/unittests/BinaryFormat/CMakeLists.txt
+++ b/llvm/unittests/BinaryFormat/CMakeLists.txt
@@ -5,6 +5,7 @@ set(LLVM_LINK_COMPONENTS
 
 add_llvm_unittest(BinaryFormatTests
   DwarfTest.cpp
+  ELFTest.cpp
   MachOTest.cpp
   MsgPackDocumentTest.cpp
   MsgPackReaderTest.cpp
diff --git a/llvm/unittests/BinaryFormat/ELFTest.cpp b/llvm/unittests/BinaryFormat/ELFTest.cpp
new file mode 100644
index 00000000000000..29793bf99c6000
--- /dev/null
+++ b/llvm/unittests/BinaryFormat/ELFTest.cpp
@@ -0,0 +1,32 @@
+//===- ELFTest.cpp ----------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/BinaryFormat/ELF.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace llvm::ELF;
+
+namespace {
+TEST(ELFTest, OSAbi) {
+  EXPECT_EQ(ELFOSABI_GNU, convertOSToOSAbi("gnu"));
+  EXPECT_EQ(ELFOSABI_FREEBSD, convertOSToOSAbi("freebsd"));
+  EXPECT_EQ(ELFOSABI_STANDALONE, convertOSToOSAbi("standalone"));
+  EXPECT_EQ(ELFOSABI_NONE, convertOSToOSAbi("none"));
+  // Test unrecognized strings.
+  EXPECT_EQ(ELFOSABI_NONE, convertOSToOSAbi(""));
+  EXPECT_EQ(ELFOSABI_NONE, convertOSToOSAbi("linux"));
+
+  EXPECT_EQ("gnu", convertOSAbiToOS(ELFOSABI_GNU));
+  EXPECT_EQ("freebsd", convertOSAbiToOS(ELFOSABI_FREEBSD));
+  EXPECT_EQ("standalone", convertOSAbiToOS(ELFOSABI_STANDALONE));
+  EXPECT_EQ("none", convertOSAbiToOS(ELFOSABI_NONE));
+  // Test unrecognized values.
+  EXPECT_EQ("none", convertOSAbiToOS(0xfe));
+}
+} // namespace

.
Created using spr 1.3.5-bogner
@MaskRay MaskRay changed the title [BinaryFormat] Adjust ELFOSABI_GNU and add unittests [BinaryFormat] Adjust OSABI functions and add unittests Apr 26, 2024
Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of nits, but otherwise LGTM.

/// Convert a OS into ELF's EI_OSABI value.
uint8_t convertOSToOSAbi(StringRef OS);
// Convert a lowercase string identifier into an OSABI value.
uint8_t convertNameToOSAbi(StringRef Name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: whilst changing the signature, now might be a good time to fix the casing of Abi to ABI, since it's an acronym and therefore should be all uppercase. Same below, including the parameter.

@@ -0,0 +1,32 @@
//===- ELFTest.cpp ----------------------------------------------*- C++ -*-===//
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I believe the C++ marker only needs to be in headers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-*- C++ -*- is indeed a rare exception, so I am removing the marker.

% rg 'C\+\+ \*-=' -l -g '*.cpp'
llvm/tools/llvm-pdbutil/PrettyFunctionDumper.cpp
llvm/tools/llvm-pdbutil/PrettyBuiltinDumper.cpp
llvm/tools/llvm-pdbutil/PrettyTypeDumper.cpp
llvm/tools/llvm-pdbutil/PrettyExternalSymbolDumper.cpp
llvm/tools/llvm-pdbutil/PrettyCompilandDumper.cpp
llvm/tools/llvm-pdbutil/PrettyTypedefDumper.cpp
llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
llvm/lib/MC/SPIRVObjectWriter.cpp

Created using spr 1.3.5-bogner
@MaskRay MaskRay merged commit a5cc951 into main Apr 29, 2024
3 of 4 checks passed
@MaskRay MaskRay deleted the users/MaskRay/spr/binaryformat-adjust-elfosabi_gnu-and-add-unittests branch April 29, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants