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

Make Call*Node#*name use the constant pool #1533

Merged
merged 1 commit into from Oct 2, 2023

Conversation

eregon
Copy link
Member

@eregon eregon commented Sep 16, 2023

src/yarp.c Outdated Show resolved Hide resolved
@eregon
Copy link
Member Author

eregon commented Sep 29, 2023

I have rebased this.
In this PR we need entries of the constant pool that come from C string literals, that doesn't supported yet, so I have worked around by using owned constants by copying the literal to a malloc-ed string.
Some tests still fail, notably the serialization ones, strangely we see unknown string types, unclear to me why:

Started
E
==============================================================================================================================
Error: test_filepath_seattlerb/index_0_opasgn.txt(Prism::ParseTest): RuntimeError: Unknown serialized string type: 5
/home/eregon/code/yarp/lib/prism/serialize.rb:161:in `load_string'
/home/eregon/code/yarp/lib/prism/serialize.rb:247:in `load_node'
/home/eregon/code/yarp/lib/prism/serialize.rb:466:in `block in load_node'
/home/eregon/code/yarp/lib/prism/serialize.rb:466:in `initialize'
/home/eregon/code/yarp/lib/prism/serialize.rb:466:in `new'
/home/eregon/code/yarp/lib/prism/serialize.rb:466:in `load_node'
/home/eregon/code/yarp/lib/prism/serialize.rb:430:in `load_node'
/home/eregon/code/yarp/lib/prism/serialize.rb:112:in `load_nodes'
/home/eregon/code/yarp/lib/prism/serialize.rb:116:in `load_result'
/home/eregon/code/yarp/lib/prism/serialize.rb:31:in `load'
/home/eregon/code/yarp/lib/prism.rb:46:in `load'
/home/eregon/code/yarp/test/prism/parse_test.rb:131:in `block (2 levels) in <class:ParseTest>'
==============================================================================================================================
E
==============================================================================================================================
Error: test_filepath_seattlerb/op_asgn_dot_ident_command_call.txt(Prism::ParseTest): RuntimeError: Unknown serialized string type: 4
/home/eregon/code/yarp/lib/prism/serialize.rb:161:in `load_string'
/home/eregon/code/yarp/lib/prism/serialize.rb:249:in `load_node'
/home/eregon/code/yarp/lib/prism/serialize.rb:466:in `block in load_node'
/home/eregon/code/yarp/lib/prism/serialize.rb:466:in `initialize'
/home/eregon/code/yarp/lib/prism/serialize.rb:466:in `new'
/home/eregon/code/yarp/lib/prism/serialize.rb:466:in `load_node'
/home/eregon/code/yarp/lib/prism/serialize.rb:430:in `load_node'
/home/eregon/code/yarp/lib/prism/serialize.rb:112:in `load_nodes'
/home/eregon/code/yarp/lib/prism/serialize.rb:116:in `load_result'
/home/eregon/code/yarp/lib/prism/serialize.rb:31:in `load'
/home/eregon/code/yarp/lib/prism.rb:46:in `load'
/home/eregon/code/yarp/test/prism/parse_test.rb:131:in `block (2 levels) in <class:ParseTest>'
==============================================================================================================================
E
==============================================================================================================================
Error: test_filepath_seattlerb/op_asgn_index_command_call.txt(Prism::ParseTest): RuntimeError: Unknown serialized string type: 6
/home/eregon/code/yarp/lib/prism/serialize.rb:161:in `load_string'
/home/eregon/code/yarp/lib/prism/serialize.rb:249:in `load_node'
/home/eregon/code/yarp/lib/prism/serialize.rb:466:in `block in load_node'
/home/eregon/code/yarp/lib/prism/serialize.rb:466:in `initialize'
/home/eregon/code/yarp/lib/prism/serialize.rb:466:in `new'
/home/eregon/code/yarp/lib/prism/serialize.rb:466:in `load_node'
/home/eregon/code/yarp/lib/prism/serialize.rb:430:in `load_node'
/home/eregon/code/yarp/lib/prism/serialize.rb:112:in `load_nodes'
/home/eregon/code/yarp/lib/prism/serialize.rb:116:in `load_result'
/home/eregon/code/yarp/lib/prism/serialize.rb:31:in `load'
/home/eregon/code/yarp/lib/prism.rb:46:in `load'
/home/eregon/code/yarp/test/prism/parse_test.rb:131:in `block (2 levels) in <class:ParseTest>'
==============================================================================================================================
/ruby: src/serialize.c:52: pm_serialize_string: Assertion `false && "Cannot serialize mapped strings."' failed.
rake aborted!
SignalException: SIGABRT

We also see /ruby: src/serialize.c:52: pm_serialize_string: Assertion false && "Cannot serialize mapped strings."' failed.`.

@eregon eregon force-pushed the call-node-name-constant branch 2 times, most recently from 9e6d042 to 59e0d61 Compare September 29, 2023 22:26
@eregon
Copy link
Member Author

eregon commented Sep 29, 2023

OK I found the issue, the problem was:

pm_call_and_write_node_create(pm_parser_t *parser, pm_call_node_t *target, const pm_token_t *operator, pm_node_t *value) {
...
    *node = (pm_call_and_write_node_t) {
...
        .write_name = target->name,

And the C compiler says nothing here, even though it's assigning a pm_constant_id_t=uint32_t to a pm_string_t (a struct with 3 fields). WTF! It turns out if I change pm_string_t to not have the enum as the first field then it does error, so I did that.
Maybe we should type pm_constant_id_t as not just uint32_t but a struct with a single field, that might tell the compiler those types are incompatible and a pm_constant_id_t is not just "any C integer, possibly an enum value".

@eregon eregon changed the title Make CallNode#name use the constant pool Make Call*Node#*name use the constant pool Sep 29, 2023
@eregon
Copy link
Member Author

eregon commented Sep 29, 2023

OK this is ready for review.
I did all the Call*Node#*name fields, since they are connected and it doesn't make sense to do them individually (e.g. would require yp_constant_id to yp_string_t conversion which is hard/slow/unnecessary otherwise).

  • CallAndWriteNode read_name, write_name
  • CallNode name
  • CallOperatorWriteNode read_name, write_name
  • CallOrWriteNode read_name, write_name

Comment on lines +442 to +454
pm_parser_constant_id_static(pm_parser_t *parser, const char *start, size_t length) {
uint8_t *owned_copy;
if (length > 0) {
owned_copy = malloc(length);
memcpy(owned_copy, start, length);
} else {
owned_copy = malloc(1);
owned_copy[0] = '\0';
}
return pm_constant_pool_insert_owned(&parser->constant_pool, owned_copy, length);
// Does not work because the static literal cannot be serialized as an offset of source
// return pm_constant_pool_insert_shared(&parser->constant_pool, start, length);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I think once we always serialize all constants as embedded then we can simplify this and actually use the C literal string pointer without copying, because then we only need start+offset, and don't care if that's within the source or not when serializing.

@eregon
Copy link
Member Author

eregon commented Sep 30, 2023

CI is green now, after ruby-head has been rebuilt with the latest fixes.

eregon added a commit to eregon/ruby that referenced this pull request Oct 1, 2023
@eregon
Copy link
Member Author

eregon commented Oct 1, 2023

I also made the PR to adopt the change in CRuby: ruby/ruby#8573

kddnewton pushed a commit to ruby/ruby that referenced this pull request Oct 2, 2023
@kddnewton kddnewton merged commit 850efd6 into ruby:main Oct 2, 2023
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants