-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: add Object registry for foreign objects and binary_tree module #12
base: main
Are you sure you want to change the base?
Conversation
…at/js-object-registry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to add some tests to the tests
folder too!
parameters: [ | ||
{ | ||
type: "primary", | ||
primaryDataType: "signed int" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do specify somewhere in the docs that this defers from normal make_tree
source academy module as you are only able to take in signed ints as your node value, whereas the module itself could have any JS type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I will talk about this with Professor. Since they would have to pass an address or literal value I might change this to accept an address and another method for literal values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I already wanted to add new tests for binary_tree but writing tests for modules that are implemented using foreign objects are not possible since the modules definitions are passed to c-slang using frontend. Do you have any suggestions ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either:
- you just write code samples that can be run in the source academy frontend, similar to what i did for pix_n_flix, under test/samples/module_test_samples, and manually test them to ensure you have a fully functioning module (with the bonus of giving any users of this module some sample code that they can refer to). This is the easiest, but is not an automated solution.
- (better if possible) seems that the binary_tree source academy module does not depend on any browser environment specific APIs (unlike pix_n_flix), so you might be able to simply load in the binary_tree module in a NodeJS environment, which is what the automated unit tests run in. Once you have the module functions imported, you should be able to provide the functions under the
externalFunctions
of themodulesConfig
provided tocompileAndRunFile()
here:Lines 154 to 163 in 5af7e57
const modulesConfig = { printFunction: (str) => programOutput.push(str), // custom print function, add to the programOutput instead of print to console }; try { // if there is a expectedValues for variables in the file, check that they are equal await compileAndRunFile({ testGroup, testFileName, modulesConfig, });
Then your test samples should work in the unit tests just the same as if running in source acedemy frontend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I will add some test samples for binary_test. For your second suggestion I would have to import binary tree module by cloning the modules or only the binary_tree implementation in c-slang to pass its functions instead of using the modules served in frontend (correct me if I am wrong) and wouldn't this make them inconsistent since then we might be relying on two different sources if modules are changed in the frontend ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think the binary_tree module is going to change anytime, you should be fine just importing the module functions
ctowasm/src/modules/util.ts
Outdated
} | ||
const objIdentifier = new Uint8Array([ | ||
(FOREIGN_OBJ_IDENTIFIER >> 8) & 0xFF, // High byte (0xF0) | ||
FOREIGN_OBJ_IDENTIFIER & 0xFF // Low byte (0xBA) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the purpose of FOREIGN_OBJ_IDENTIFIER ? it does not seem to be used anywhere. It seems you're just using it as a placeholder value for the address that you give your foreign object for the value of its void *
. In that case you could just leave it as zeros since from the C code deferencing the void *
would be UB. Unless you need it on the JS side, which dosent seem to be the case in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively the C standard does not specify that the value of a pointer must be an actual address in memory, merely a "reference". In that case you could instead come up with a solution to giving a unique value for your foreign object void *
variables which are guaranteed to not be equal to any other foreign object or actual C object or NULL. To do this, you could reserve a portion of the addressable memory range beforehand and assign foreign object void *
values from there, or even just increase the underlying primitive type of pointers from unsigned int
to unsigned long int
(32bit to 64bit), while the WASM addressable space is still 32bit (limitation of WASM memory addressing AFAIK). You could modify this in c-slang/ctowasm/src/common/constants.ts (might be some other changes needed too). That way you can just assign your foreign object void *
values from outside the unsigned 32bit numeric range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate for the review. Yeah I wanted to have a placeholder value and I thought I would use that value later in the JS side to check if the block is reserved for foreign objects or not but this is not relevant anymore since I can already check the address in the objectReferenceRegistry. I already changed it to 0.
The solution you suggested regarding using long int values for unique identifiers is nice but I think it might make it a little confusing for learners when they try to print address and see that it holds such a big number while being not related to the memory structure and address we are using. And it might be a little over engineering if we can solve this problem with reserving only one byte of address from our current memory don't you think so ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"print address" for a foreign object has no meaning to begin with, as the object itself does not even exist within the the 32bit memory range we are working. In fact it is incorrect to even call the value of the void *
variable referring to the foreign object an "address" to begin with. I would prefer the term reference, as it is a reference to an object that exists outside of your memory space. This fits nicely with the C standard.
As for "over-engineering", the solution i proposed should not be very difficult and require minimal changes. There are other things you need to consider as well:
Everytime you get a new foreign object you allocate 1 byte of the addressable 32bit memory as the "address" of that of that object. However how do you free that space if you no longer need the object? I see you modified the freeFunction()
in memory.ts
to remove the foreign object's "address" from memory space if it is called:
c-slang/ctowasm/src/modules/source_stdlib/memory.ts
Lines 94 to 97 in f9372a6
// remove the object from the object reference registry (if present) | |
if (objectReferenceRegistry.get(address)) { | |
objectReferenceRegistry.delete(address); | |
} |
make_tree
, but then if you want to remove the space it uses you need to call free()
in your C program. Do you see the problem here? The user would need to call free()
on a pointer that is not NULL or a pointer returned from a call from malloc()
, calloc()
etc, which is UB. I don't think we want to encourage students to write C programs with obvious UB in them. And if we never call free()
then that placeholder byte that "address" references is gone for good, regardless of whether we need the foreign object. I see two responses to this:
- You remove the abilty to remove foreign objects using
free()
entirely, and trust students to never have so many foreign objects that they still have enough addressable memory for their programs. - Return a reference to a value that lies outside the addressable memory range like I suggested.
I think you should discuss with Prof Henz which would be preferable. Finally one more thing to note about your existing solution is that when you use foreign objects, your heap segment will be populate with these 0 bytes, so in the event people are printing out the heap segment to analyze memory (which sometimes happens depending on what you are studying/want to achieve), these 0 bytes may be misleading/a distraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your detailed answer. Yeah I agree that calling free on foreign object is UB and can be confusing and misleading for students. I will probably proceed with your solution to use references outside the addressable memory range. One more thing is that at the end of the day even if we give them a reference that is outside the addressable memory we are still storing the foreign objects in the JS side and we don't have garbage collector or anything similar to delete the foreign objects that are out of scope or unusable anymore. So we are still trusting users to not create many foreign objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, the difference is that each foreign object now doesn't permenantly take up space in the memory addressable by your C programs, which is fixed
Features:
ATTENTION:
This PL should be merged after #10 and #11 .
If needed, rebase it first with main branch then merge it.
Tests:
All of the tests are passed.