Branch: refs/heads/main
Revision: 19b9abe
Author: bradcray
Link: Avoid type queries of LHS in ChapelHashtable._moveInit() by bradcray · Pull Request #18659 · chapel-lang/chapel · GitHub
Log Message:
Merge pull request #18659 from bradcray/move-init-no-type-check
Refactor Hash Tables to use Memory.Initialization and address runtime type issues
[reviewed by @mppf and @bmcdonald3 (an early draft)]
The copies of _moveInit() in ChapelHashtable.chpl have been doing
type queries on the lhs expression, both to make sure it matches the
type of the rhs expression (i.e., 'lhs.type == rhs.type') and to test
against 'nothing' (i.e., 'lhs.type == nothing'). However, this is
problematic if the 'lhs' has a runtime type (e.g., is an array or domain),
since the '.type' queries try to compute its runtime type, but by definition
the 'lhs' in a _moveInit() is uninitialized. When --dead-code-elimination
is on, the runtime type check never made it through to code generation,
but with --baseline, which turns it off, we'd try to query the lhs
runtime type, and end up reading uninitialized memory.
This PR refactors the Hash Table implementation to use moveInitialize()
from Memory.Initialization instead (which had the same weakness). It
then refactors the moveInitialization() routines to use only the static
type information (via a primitive—thanks to Michael for reminding me
about it) to avoid this issue. Note that this problem and question are
tangled up in a longstanding discussion topic about whether Chapel
should support distinct "static type" vs. "dynamic type" queries (e.g.,
myExpr.stype()).
Since this change removes the self-documenting nature of the formal
argument list, I updated the documentation to mention that the two
expressions must be of the same type. I also added a user-facing
error message when the types do not match (where, previously, it would
have simply been an unresolved call).
Not needed for the sake of correctness, it also replaces the local
ChapelHashtable.chpl-specific _moveToReturn() routine with
moveToValue() from Memory.Initialization.
I added some tests of the new error messages, and updated other
tests that reflect the module initialization orders and the like. I also
added a test that uses --baseline so that if this logic breaks in
the future we'll catch it before baseline testing runs in the nightlies.
One slightly unsatisfying thing about this change is
that I don't have a good explanation for why the runtime type check is
unused in the generated --baseline code or what we could do to avoid
inserting it in the first place, since it doesn't seem used/useful.
Resolves https://github.com/Cray/chapel-private/issues/2644
Modified Files:
A test/library/standard/Memory/Initialization/moveInitBad.chpl
A test/library/standard/Memory/Initialization/moveInitBad.compopts
A test/library/standard/Memory/Initialization/moveInitBad.good
A test/library/standard/Memory/Initialization/moveNothing.chpl
A test/library/standard/Memory/Initialization/moveNothing.good
A test/library/standard/Memory/Initialization/setRTTypeBaseline.chpl
A test/library/standard/Memory/Initialization/setRTTypeBaseline.compopts
A test/library/standard/Memory/Initialization/setRTTypeBaseline.good
M modules/internal/ChapelHashtable.chpl
M modules/standard/Memory/Initialization.chpl
M test/compflags/ferguson/print-module-resolution.good
M test/modules/bradc/printModStuff/foo.good
M test/modules/sungeun/init/printModuleInitOrder.good
M test/modules/sungeun/init/printModuleInitOrder.na-none.good
Compare: https://github.com/chapel-lang/chapel/compare/033925eacb8f...19b9abe499cb