Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print hooked sort elements correctly #914

Merged
merged 5 commits into from
Dec 4, 2023
Merged

Conversation

Baltoli
Copy link
Contributor

@Baltoli Baltoli commented Dec 4, 2023

This PR changes the way that we print and serialize collection elements. Previously, we assumed that the elements of collections were uniformly of sort KItem. This is indeed the case for the built-in Map, Set and List sorts, but the assumption breaks down in the presence of custom collection sorts that sort their elements differently (for example, defining a custom Map that requires its keys to only be integers). Terms of these custom sorts are printed incorrectly, with spurious injections into KItem rather than their actual element sorts.

The solution to the problem is to look up the argument sorts of the element constructor when printing and serializing collections; the required information is already present in the binary and just requires a bit of refactoring to access.

This PR makes the following changes:

  • Adds @geo2a's minimised test case from Custom hooked Map's element constructor is stripped of sort injection #886 as a new integration test.
  • Refactors the generation of argument and return sort tables when the configuration parser is emitted; previously these did not use the emitDataTable infrastructure and so required custom C functions to be written to index into them.
  • Look up element sorts using the newly-accessible table when printing collections.

Fixes #886

@Baltoli Baltoli force-pushed the hooked-sort-support-v2 branch from 9ac2a8a to db0a602 Compare December 4, 2023 14:18
@Baltoli Baltoli marked this pull request as ready for review December 4, 2023 14:32
@gtrepta
Copy link
Contributor

gtrepta commented Dec 4, 2023

Just noticed this

c09b883#diff-3bc7cdc99077e33c16a7006a9027797a7bb1ae48cd6fbb22117987db7e0e2da6L1334-L1338

It looks like str_type doesn't include the "sort_name_" characters in the array size. I'm assuming it should.

Copy link
Collaborator

@theo25 theo25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me

@Baltoli
Copy link
Contributor Author

Baltoli commented Dec 4, 2023

@gtrepta The sort_name_ part is only in the LLVM variable name; e.g.:

@sort_name_SortList = global [9 x i8] c"SortList\00"

Here the type is [9 x i8] and "SortList\00" is 9 characters long as we expect.

@rv-jenkins rv-jenkins merged commit f43af65 into master Dec 4, 2023
5 checks passed
@rv-jenkins rv-jenkins deleted the hooked-sort-support-v2 branch December 4, 2023 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom hooked Map's element constructor is stripped of sort injection
4 participants