[Object] Provide operator< for ELFSymbolRef

[aaupov]

Normally, operator< accepting DataRefImpl is used when comparing
SymbolRef/ELFSymbolRef. However, it uses std::memcmp which interprets
DataRefImpl union as char string so that the result depends on host endianness.
For ELFSymbolRef a specialized operator< can be used instead to produce
consistent ordering regardless of endianness by comparing the symbol table index
and symbol index fields separately.

[llvmbot]

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

Author: Amir Ayupov (aaupov)

Changes

Normally, operator< accepting DataRefImpl is used when comparing
SymbolRef/ELFSymbolRef. However, it uses std::memcmp which interprets
DataRefImpl union as char string. For ELFSymbolRef it's known that it
uses the struct view of the union, therefore a specialized operator< can
be used instead.

---

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

1 Files Affected:

• (modified) llvm/include/llvm/Object/ELFObjectFile.h (+8)

diff --git a/llvm/include/llvm/Object/ELFObjectFile.h b/llvm/include/llvm/Object/ELFObjectFile.h
index 1d457be93741f2..8ec0e3fe15dc9d 100644
--- a/llvm/include/llvm/Object/ELFObjectFile.h
+++ b/llvm/include/llvm/Object/ELFObjectFile.h
@@ -199,6 +199,14 @@ class ELFSymbolRef : public SymbolRef {
   }
 };
 
+inline bool operator<(const ELFSymbolRef &A, const ELFSymbolRef &B) {
+  const DataRefImpl &DRIA = A.getRawDataRefImpl();
+  const DataRefImpl &DRIB = B.getRawDataRefImpl();
+  if (DRIA.d.a == DRIB.d.a)
+    return DRIA.d.b < DRIB.d.b;
+  return DRIA.d.a < DRIB.d.b;
+}
+
 class elf_symbol_iterator : public symbol_iterator {
 public:
   elf_symbol_iterator(const basic_symbol_iterator &B)

[jh7370]

I think this could use some simple unit tests in the ELFObjectFileTest.cppp file.

[jh7370]

I've not really looked myself, but are there instances where you actually have two ELFSymbolRef objects that are compared (using <)? If not, this code is dead, I believe. Also, is there a reason you provide < but not ==? I note that DataRefImpl has both.

[aaupov]

I've not really looked myself, but are there instances where you actually have two ELFSymbolRef objects that are compared (using <)? If not, this code is dead, I believe.

I'm going to use it in FILE symbol lookup using upper_bound on ELFSymbolRefs here, instead of using DRI.d.b (symbol index) directly: #89648

Also, is there a reason you provide < but not ==? I note that DataRefImpl has both.

DataRefImpl's operator== is good enough for comparison.

[jh7370]

Also, is there a reason you provide < but not ==? I note that DataRefImpl has both.

DataRefImpl's operator== is good enough for comparison.

Why is DataRefImpl's operator< not good enough when the operator== is?

Also, please try to avoid force pushes in active reviews, per the LLVM guidelines. If rebasing is necessary, consider merging main into your branch instead.

[aaupov]

Also, is there a reason you provide < but not ==? I note that DataRefImpl has both.

DataRefImpl's operator== is good enough for comparison.

Why is DataRefImpl's operator< not good enough when the operator== is?

Sorry, I've made the commit message more clear. DataRefImpl's operator< uses std::memcmp whose result is endianness-dependent (as shown by added test), which is exactly the issue addressed by a custom operator< for ELFSymbolRefs.
operator== also uses std::memcmp but equality is invariant wrt endianness.

Also, please try to avoid force pushes in active reviews, per the LLVM guidelines. If rebasing is necessary, consider merging main into your branch instead.

Noted, thank you.

[jh7370]

Thanks for the explanation about why this whole thing is important, the new commit message is certainly clearer. I think the last sentence of the description would be clearer saying:
"For ELFSymbolRef a specialized operator< can be used instead to produce consistent ordering regardless of endianness by comparing the symbol table index and symbol index fields separately."

One other thing: does DataRefImpl have an operator>? If so, you'll need that here too as otherwise two ELFSymbolRefs will not have inconsistent orderings based on which comparison operator is being used.

[aaupov]

Thanks for the explanation about why this whole thing is important, the new commit message is certainly clearer. I think the last sentence of the description would be clearer saying: "For ELFSymbolRef a specialized operator< can be used instead to produce consistent ordering regardless of endianness by comparing the symbol table index and symbol index fields separately."

Thanks, updated.

One other thing: does DataRefImpl have an operator>? If so, you'll need that here too as otherwise two ELFSymbolRefs will not have inconsistent orderings based on which comparison operator is being used.

No, it doesn't:

llvm-project/llvm/include/llvm/Object/SymbolicFile.h

Lines 53 to 67 in 2d09ac4

	inline bool operator==(const DataRefImpl &a, const DataRefImpl &b) {
	// Check bitwise identical. This is the only legal way to compare a union w/o
	// knowing which member is in use.
	return std::memcmp(&a, &b, sizeof(DataRefImpl)) == 0;
	}
	inline bool operator!=(const DataRefImpl &a, const DataRefImpl &b) {
	return !operator==(a, b);
	}
	inline bool operator<(const DataRefImpl &a, const DataRefImpl &b) {
	// Check bitwise identical. This is the only legal way to compare a union w/o
	// knowing which member is in use.
	return std::memcmp(&a, &b, sizeof(DataRefImpl)) < 0;
	}

[aaupov]

@jh7370: thank you for extensive comments, the test is a lot more readable now. Hope it's good to go now :)

[jh7370]

LGTM, with one nit.