Issue 5216 - [emscripten] Use a more reliable way of retrieving mangled function name

Issue 5216 - [emscripten] Use a more reliable way of retrieving mangled function name

[Wladimir Palant, 2017-05-08 08:53:57]

Patch Set 2 fixes the issues below.

https://codereview.adblockplus.org/29431555/diff/29431556/compiled/bindings/l...
File compiled/bindings/library.js (right):

https://codereview.adblockplus.org/29431555/diff/29431556/compiled/bindings/l...
compiled/bindings/library.js:22: GetFunctionName__sig: ["iii"],
This signature is actually wrong, it should have been "viii". It works
regardless, so it seems that the signatures specified here aren't important for
our use case. I removed them.

https://codereview.adblockplus.org/29431555/diff/29431556/compiled/bindings/l...
compiled/bindings/library.js:27: Module.stringToAscii(name, buffer);
This function doesn't need to be called via Module in this context, it can be
accessed directly.

Also, this is a potential memory safety issue as buffer size isn't checked -
better fix this while we are changing the code anyway.

[sergei, 2017-05-08 09:16:29]

https://codereview.adblockplus.org/29431555/diff/29431556/compiled/bindings/l...
File compiled/bindings/library.js (right):

https://codereview.adblockplus.org/29431555/diff/29431556/compiled/bindings/l...
compiled/bindings/library.js:22: GetFunctionName__sig: ["iii"],
On 2017/05/08 08:53:57, Wladimir Palant wrote:
> This signature is actually wrong, it should have been "viii". It works
> regardless, so it seems that the signatures specified here aren't important
for
> our use case. I removed them.

Wouldn't it be better to completely remove GetFunctionName__sig instead of
defining it as an empty array? BTW, in the examples in the repo they use string
for the signature, e.g. 'viii' instead of array.

https://codereview.adblockplus.org/29431555/diff/29433555/compiled/bindings/g...
File compiled/bindings/generator.cpp (right):

https://codereview.adblockplus.org/29431555/diff/29433555/compiled/bindings/g...
compiled/bindings/generator.cpp:142: GetFunctionName(name.data(), function,
signature.c_str());
if std::string::data() returns only a const pointer then one may use `&name[0]`.

https://codereview.adblockplus.org/29431555/diff/29433555/compiled/bindings/g...
File compiled/bindings/generator.h (right):

https://codereview.adblockplus.org/29431555/diff/29433555/compiled/bindings/g...
compiled/bindings/generator.h:122: std::vector<char> name;
We are converting it later to std::string, I think it would be better to define
it as std::string now.

[Wladimir Palant, 2017-05-08 10:17:18]

https://codereview.adblockplus.org/29431555/diff/29431556/compiled/bindings/l...
File compiled/bindings/library.js (right):

https://codereview.adblockplus.org/29431555/diff/29431556/compiled/bindings/l...
compiled/bindings/library.js:22: GetFunctionName__sig: ["iii"],
On 2017/05/08 09:16:29, sergei wrote:
> Wouldn't it be better to completely remove GetFunctionName__sig instead of
> defining it as an empty array? BTW, in the examples in the repo they use
string
> for the signature, e.g. 'viii' instead of array.

This entry is required, GetFunctionName__asm will be ignored otherwise. The
default library has both arrays and strings here. However, from what I saw in
the source code so far it appears that the value is actually unused. So maybe
worth filing an Emscripten issue on it.

https://codereview.adblockplus.org/29431555/diff/29433555/compiled/bindings/g...
File compiled/bindings/generator.cpp (right):

https://codereview.adblockplus.org/29431555/diff/29433555/compiled/bindings/g...
compiled/bindings/generator.cpp:142: GetFunctionName(name.data(), function,
signature.c_str());
On 2017/05/08 09:16:29, sergei wrote:
> if std::string::data() returns only a const pointer then one may use
`&name[0]`.

Oh, so the C++ standard in its most recent iteration finally managed to define a
non-const std::string::data() and the data buffer can now be modified in a
non-hacky way? Progress is nice, done :)

https://codereview.adblockplus.org/29431555/diff/29433555/compiled/bindings/g...
File compiled/bindings/generator.h (right):

https://codereview.adblockplus.org/29431555/diff/29433555/compiled/bindings/g...
compiled/bindings/generator.h:122: std::vector<char> name;
On 2017/05/08 09:16:29, sergei wrote:
> We are converting it later to std::string, I think it would be better to
define
> it as std::string now.

Done.

[sergei, 2017-05-08 10:58:19]

LGTM

https://codereview.adblockplus.org/29431555/diff/29433567/compile
File compile (right):

https://codereview.adblockplus.org/29431555/diff/29433567/compile#newcode78
compile:78: '-o', BINDINGS_GENERATOR, '-std=c++1z', '--js-library', JS_LIBRARY,
does c++1z influence somehow on the generated code/size/something else?

https://codereview.adblockplus.org/29431555/diff/29433567/compiled/bindings/g...
File compiled/bindings/generator.cpp (right):

https://codereview.adblockplus.org/29431555/diff/29433567/compiled/bindings/g...
compiled/bindings/generator.cpp:147: return name.size() == 0;
name.empty() would be better.

[Wladimir Palant, 2017-05-08 11:41:34]

Patch Set 4 fixes the issues below.

https://codereview.adblockplus.org/29431555/diff/29433567/compile
File compile (right):

https://codereview.adblockplus.org/29431555/diff/29433567/compile#newcode78
compile:78: '-o', BINDINGS_GENERATOR, '-std=c++1z', '--js-library', JS_LIBRARY,
On 2017/05/08 10:58:19, sergei wrote:
> does c++1z influence somehow on the generated code/size/something else?

No, the build is identical. The only thing changing is the non-const
string::data() being available.

I realized that I changed this parameter only for the bindings, not for the
final build - fixed now.

https://codereview.adblockplus.org/29431555/diff/29433567/compiled/bindings/g...
File compiled/bindings/generator.cpp (right):

https://codereview.adblockplus.org/29431555/diff/29433567/compiled/bindings/g...
compiled/bindings/generator.cpp:147: return name.size() == 0;
On 2017/05/08 10:58:19, sergei wrote:
> name.empty() would be better.

Done.

[sergei, 2017-05-08 11:48:51]

LGTM