Reconsider assert(!"Message") usage in coreclr.

[sandreenko]

• There are at least 3 asserts that are always true, because of a forgotten ! before the message, when fixed they fail on some tests, see Fix asserts that were always true due to a missed neg. #44095, PTAL @tannergooding.

• PORTABILITY_ASSERT(message) (108 usages) are quite confusing because they don't check a condition and fail always. They could be replaced by unreached or unimplemented methods, also this block looks pretty outdated:
  

  runtime/src/coreclr/src/inc/palclr.h

  Lines 103 to 111 in df8e3f6

  	#if defined(TARGET_X86)
  	// Finished ports - compile-time errors
  	#define PORTABILITY_WARNING(message) NEED_TO_PORT_THIS_ONE(NEED_TO_PORT_THIS_ONE)
  	#define PORTABILITY_ASSERT(message) NEED_TO_PORT_THIS_ONE(NEED_TO_PORT_THIS_ONE)
  	#else
  	// Ports in progress - run-time asserts only
  	#define PORTABILITY_WARNING(message)
  	#define PORTABILITY_ASSERT(message) _ASSERTE(false && (message))
  	#endif

  
  are not other platforms finished now?
  PTAL @BruceForstall, @davidwrighton, you had a similar discussion about IMPL_LIMIT recently, do you have preferences about assert/unimplemented/unreached usages?

• There are 6 usages of ASSERT(!"message") and 360 usages of ASSERT("message") with different defines, probably replace both with better debug only unreached("message")?

cc @dotnet/jit-contrib

[jkotas]

PORTABILITY_ASSERT(message)

The purpose of these is to mark places that need to be filled in for new platform ports. I do not think we should replace them with the generic unreached. We would lose the purpose. Renaming them to reduce confusion sounds good.

this block looks pretty outdated

Yes, this shoud be flipped to #if 1 since we do not have any platform ports in progress currently, and anything that does not compile fixed.

[sandreenko]

Yes, this shoud be flipped to #if 1 since we do not have any platform ports in progress currently, and anything that does not compile fixed.

I have seen only 2 failures so far:

runtime/src/coreclr/src/debug/di/valuehome.cpp

Lines 454 to 457 in ecd979c

	// TODO: : implement CordbValue::SetEnregisteredValue for RAK_FLOAT
	#if defined(TARGET_AMD64)
	PORTABILITY_ASSERT("NYI: SetEnregisteredValue (divalue.cpp): RAK_FLOAT for AMD64");
	#endif // TARGET_AMD64

runtime/src/coreclr/src/vm/threadsuspend.cpp

Lines 5128 to 5144 in ecd979c

	#if defined(TARGET_X86) && !defined(FEATURE_EH_FUNCLETS)
	// Second pass, looking for the address of the return address so we can
	// hijack:
	PREGDISPLAY pRDT = pCF->GetRegisterSet();
	if (pRDT != NULL)
	{
	// pPC points to the return address sitting on the stack, as our
	// current EIP for the penultimate stack frame.
	pES->m_ppvRetAddrPtr = (void **) pRDT->PCTAddr;
	STRESS_LOG2(LF_SYNC, LL_INFO1000, "Partially Int case hijack address = 0x%x val = 0x%x\n", pES->m_ppvRetAddrPtr, *pES->m_ppvRetAddrPtr);
	}
	#else
	PORTABILITY_ASSERT("Platform NYI");
	#endif

[jkotas]

threadsuspend.cpp: The ifdef should be changed to just #if !defined(FEATURE_EH_FUNCLETS) and the else part of the ifdef should be deleted.

Not sure about the right fix for valuehome.cpp.

[davidwrighton]

@sandreenko, looking at the valuehome code it's doing save/restore of the x87 registers. I believe we don't use them anymore for x86 codegen, and instead use the SSE registers. I suppose it might be there to support mixed mode debugging, but I don't have confidence in that. I would guess that this code in theory is used to modify register state in the registers window, or possibly to handle some optimized debugging scenarios that I suspect don't work very well.

@tommcdon might have some context.

[davidwrighton]

Also my general opinion is that a better name would be good, but something like unreached causes a bunch of compiler warning spew that is different between various compilers and optimization levels. I'd like to see what sort of changes are needed to get a clean PR build before having an opinion on that.

[tannergooding]

I believe we don't use them anymore for x86 codegen, and instead use the SSE registers

The x86 (but not x64) ABI still requires floating-point values to be returned in ST(0), so maybe it needs to account for that?

[tommcdon]

Adding @sdmaclea to comment on the debugger ARM64 NYI assert

runtime/src/coreclr/src/debug/di/valuehome.cpp

Lines 454 to 457 in ecd979c

	// TODO: : implement CordbValue::SetEnregisteredValue for RAK_FLOAT
	#if defined(TARGET_AMD64)
	PORTABILITY_ASSERT("NYI: SetEnregisteredValue (divalue.cpp): RAK_FLOAT for AMD64");
	#endif // TARGET_AMD64

[BruceForstall]

There are 6 usages of ASSERT(!"message") and 360 usages of ASSERT("message") with different defines, probably replace both with better debug only unreached("message")?

The ASSERT("message") cases are all in the PAL, which is "unique". The VM cases of ASSERT(!"message") maybe could be changed to use _ASSERTE like most everything else. It's not clear there's value in making it unreached, which as David says can cause warning problems with various compilers.