clean up assert for machine_implc.cc

[eddy16112]

I proposed four ways to clean up four different types of assert.

1. The assert contains a function call that can not be removed if NDEBUG is enabled
   e. g.

assert(ptr->add_process_info(process_info));

replace it into

if (!ptr->add_process_info(process_info)) {abort with some error messages;}

2. assert(0) or assert(false);
   replace it with abort;
3. assert that can be handled with error code by callers. This is usually for functions that will be exposed to users.
   e.g.
   assert inside the

MachineImpl::get_local_processors_by_kind(std::set<Processor> &pset, Processor::Kind kind) const

instead of assert, we can let the function returns an error code and let the caller to handle it.
4. other assert
Replace them with REALM_ASSERT_WITH_ABORT(), a macro that print the error and abort.

[codecov]

Codecov Report

❌ Patch coverage is 6.52174% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 27.15%. Comparing base (ade8156) to head (fbd4005).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/realm/machine_impl.cc	2.43%	28 Missing and 12 partials ⚠️
src/realm/sparsity.inl	50.00%	2 Missing ⚠️
src/realm/indexspace.inl	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #327      +/-   ##
==========================================
- Coverage   27.15%   27.15%   -0.01%     
==========================================
  Files         190      190              
  Lines       39174    39178       +4     
  Branches    14289    14196      -93     
==========================================
- Hits        10638    10637       -1     
+ Misses      27681    27356     -325     
- Partials      855     1185     +330     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[muraj]

Can we name this differently? Something like REALM_BUG_ON or something (following the linux kernel naming for something similar).

[apryakhin]

agree with @muraj but I don't like REALM_BUG_on either. Perhaps...REALM_ABORT_ON?

[muraj]

we shouldn't be removing these. Assert is perfectly fine to use as documentation and debugging, but it should not be used on code that is expected to run in release (i.e. when asserts are turned off).

A much easier way to manage what we need is to do something like the following instead (this is what CUDA does):

#ifdef NDEBUG
#define REALM_ASSERT(x) if(!(x)) { log_runtime.fatal("%s(%d): failed assert '%s'", __FILE__, __LINE__, #x); abort(); }
#else
#define REALM_ASSERT(x) assert(x)
#endif

Then replace all uses of assert() with REALM_ASSERT(). We can then run through at a later time and clean things up as we go through to ensure the usages don't have side effects, therefore we can get rid of the extra logging stuff.

[eddy16112]

I was planing to use something like your proposed REALM_ASSERT, but then assert can not be optimized even when NDEBUG is enabled. I am wondering how to decide whether an assert can be safely optimized/ignored in release mode.

[muraj]

We're currently in a situation where we're always building with NDEBUG off. So it's not a regression to do this. We can always optimize later, let's just fix the original issue in a way we can control first, then optimize later.

[apryakhin]

Please don't remove any asserts that are unrelated to assert(0). Can we just focus on getting the assert(0) replaced with aborts. All other asserts need to more carefully reviewed and I don't want to waste time on going over everything in a single PR. That will take longer.

[eddy16112]

I did not remove any asserts. I just moved it to the upper caller for a better coverage.

Can we just focus on getting the assert(0) replaced with aborts.

The direct reason to clean up the assert is because when the NDEBUG is added (We explicitly removed the NDBUG from the cmake, but it could be added by others), realm could crash, and I have seen it before with the legete conda env on the aarch64. Anyway, I think I will just try Cory's suggestion mentioned above.

[apryakhin]

@eddy16112 Can you please undo all the changes here. There are lot of assert(0) across the codebase. I suggest let's handle those first please.

[eddy16112]

I replaced them with REALM_ASSERT