Fix for issue #82 - duplicate symbol error: memset, memcpy, memove an… #83
Conversation
…y, memove and memcmp
|
It looks like @dforsten signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
|
Does it actually help with the issue? |
| ext_memset(dest, c, n) | ||
| } | ||
| } else { | ||
| extern crate rlibc; |
There was a problem hiding this comment.
I think only this line should be removed, we'd better keep external memory ops for libc
There was a problem hiding this comment.
I tried it locally and that single-line change alone fixes the link issue as well.
Should I make a new PR, or commit a revert?
There was a problem hiding this comment.
Oh, you can update this PR since it's not merged :)
There was a problem hiding this comment.
Makes sense.
I think it would make sense to move this code into the top-level crate also (as a separate mod, but not a crate). I don't see any reason in the current granularity. Have a separated pwasm-libc crate for the purpose it serves currently has not too much sense to me.
There was a problem hiding this comment.
Ah, yeah, it is a special feature, then we can probably comment this line indeed.
However, I'm having a hard time to understand/remember why would we need this feature in the first place. Substrate doesn't use these, nor these ext_* functions are provided by the pwasm runtime.
(That said, I don't have any objections from merging this as is)
There was a problem hiding this comment.
Change committed and passed the tests!
There was a problem hiding this comment.
Discussions about refactoring the code should probably be done outside of this immediate bug fix PR. Right now I am just very eager to start multiplying U256 values in my pwasm contracts 😜.
Yes, it did fix the link errors described in issue #82 for me. |
|
Is there anything I can do to get this PR over the finish line? |
|
Thank you! |
Awesome, thanks for merging! :-) |
|
Is there an estimate when this fix will be available on crates.io? I understand it would require version bumps to pwasm-abi and pwasm-ethereum as well, so I am fine with using forks for now if that is not planned at this point. |
i did a minor update of pwasm-libc, so you can do |
Fix for issue #82
Multiplying two U256 values in a pwasm contract caused duplicate symbol link errors as a conflict between compiler_builtins and rlibc.