[lld] Remove const qualifier on symbolKind (NFC)

[nikic]

The symbol including this member is being overwritten here:

llvm-project/lld/COFF/SymbolTable.cpp

Lines 496 to 509 in 2117677

	if (Defined *d = undef->getWeakAlias()) {
	// We want to replace Sym with D. However, we can't just blindly
	// copy sizeof(SymbolUnion) bytes from D to Sym because D may be an
	// internal symbol, and internal symbols are stored as "unparented"
	// Symbols. For that reason we need to check which type of symbol we
	// are dealing with and copy the correct number of bytes.
	if (isa<DefinedRegular>(d))
	memcpy(sym, d, sizeof(DefinedRegular));
	else if (isa<DefinedAbsolute>(d))
	memcpy(sym, d, sizeof(DefinedAbsolute));
	else
	memcpy(sym, d, sizeof(SymbolUnion));
	continue;
	}

I don't think that's compatible with this const qualifier.

[llvmbot]

@llvm/pr-subscribers-platform-windows
@llvm/pr-subscribers-lld-coff

@llvm/pr-subscribers-lld

Author: Nikita Popov (nikic)

Changes

The symbol including this member is being overwritten here:

llvm-project/lld/COFF/SymbolTable.cpp

Lines 496 to 509 in 2117677

	if (Defined *d = undef->getWeakAlias()) {
	// We want to replace Sym with D. However, we can't just blindly
	// copy sizeof(SymbolUnion) bytes from D to Sym because D may be an
	// internal symbol, and internal symbols are stored as "unparented"
	// Symbols. For that reason we need to check which type of symbol we
	// are dealing with and copy the correct number of bytes.
	if (isa<DefinedRegular>(d))
	memcpy(sym, d, sizeof(DefinedRegular));
	else if (isa<DefinedAbsolute>(d))
	memcpy(sym, d, sizeof(DefinedAbsolute));
	else
	memcpy(sym, d, sizeof(SymbolUnion));
	continue;
	}

I don't think that's compatible with this const qualifier.

---

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

1 Files Affected:

• (modified) lld/COFF/Symbols.h (+1-1)

diff --git a/lld/COFF/Symbols.h b/lld/COFF/Symbols.h
index ca69fb2d05270..5ef46f5af6a6c 100644
--- a/lld/COFF/Symbols.h
+++ b/lld/COFF/Symbols.h
@@ -106,7 +106,7 @@ class Symbol {
            "If the name is empty, the Symbol must be a DefinedCOFF.");
   }
 
-  const unsigned symbolKind : 8;
+  unsigned symbolKind : 8;
   unsigned isExternal : 1;
 
 public:

[rnk]

This seems fine, but some of the subclasses still have vtables, so we're still effectively doing some kind of placement new operation here without updating the other existing Symbol references, and I think that is technically UB without more laundering operations.

[MaskRay]

The symbol including this member is being overwritten here:

llvm-project/lld/COFF/SymbolTable.cpp

Lines 496 to 509 in 2117677

	if (Defined *d = undef->getWeakAlias()) {
	// We want to replace Sym with D. However, we can't just blindly
	// copy sizeof(SymbolUnion) bytes from D to Sym because D may be an
	// internal symbol, and internal symbols are stored as "unparented"
	// Symbols. For that reason we need to check which type of symbol we
	// are dealing with and copy the correct number of bytes.
	if (isa<DefinedRegular>(d))
	memcpy(sym, d, sizeof(DefinedRegular));
	else if (isa<DefinedAbsolute>(d))
	memcpy(sym, d, sizeof(DefinedAbsolute));
	else
	memcpy(sym, d, sizeof(SymbolUnion));
	continue;
	}

I don't think that's compatible with this const qualifier.

I recall this discussion
https://discourse.llvm.org/t/can-we-add-invariant-group-for-const-class-members/75465

@zygoloid : Is the const use UB? Even if no, should this specific use be discouraged?

[zygoloid]

@zygoloid : Is the const use UB? Even if no, should this specific use be discouraged?

The relevant rule was changed for C++20; now const only fully disallows modification for complete objects, and const on data members doesn't actually mean the member is immutable any more. [Edit: I should add, this was a DR so should be expected to apply retroactively.]

Regardless I think it's best to avoid const on non-static data members in general (because it prevent assignment) and especially for ones that are actually modified -- it looks to me like this memcpy is effectively an assignment).