Skip to content

Conversation

@quantum5
Copy link
Member

@quantum5 quantum5 commented May 30, 2019

This seems sufficiently stable for an initial version.

The bullet, poppler, sqlite3 tests all run fine under LSan, without false positives. Leaks were introduced into sqlite3 and it was caught by LSan.

Limitations:

def do_smart_test(self, source, name='test.cpp', literals=[], regexes=[],
emcc_args=[], run_args=[], assert_returncode=0):
shutil.copyfile(source, name)
run_process([PYTHON, EMCC, name] + emcc_args)
Copy link
Collaborator

Choose a reason for hiding this comment

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

self.get_emcc_args() is better I think

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 don't think anything in test_other.py uses self.emcc_args.

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 recall that @kripken wants no tests in test_other to use self.emcc_args and the mechanism behind that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe that is incorrect. For self.set_setting() and self.emcc_args shouldn't work for all test subclasses. Thats the point of the common code in runner.py. @kripken can you confirm?

The set_setting/get_setting/emcc_args/build stuff in RunnerCore is all generic right?



class libubsan_rt_wasm(SanitizerLibrary):
name = 'libubsan_rt_wasm'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do these libraries need to end in _wasm? I think think that was only need when we have certain parts a library that were wasm only. In these cases I don't think its needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, the entire library is wasm only. I think we can wait until we get rid of fastcomp to get rid of the _wasm suffix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also note that this is already in the code. I don't think we should change the name of all sanitizer libraries in this change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, lets do that as a followup perhaps.

@quantum5 quantum5 requested a review from kripken June 25, 2019 17:49
@quantum5 quantum5 requested a review from sbc100 June 27, 2019 20:28
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Only had a quick glance, but seems reasonable. I'll defer to @tlively.



class libubsan_rt_wasm(SanitizerLibrary):
name = 'libubsan_rt_wasm'
Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, lets do that as a followup perhaps.

}

void DoStopTheWorld(StopTheWorldCallback callback, void *argument) {
StopTheWorld(callback, argument);
Copy link
Member

Choose a reason for hiding this comment

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

Comment that this is a nop please!

@quantum5 quantum5 merged commit 769cee7 into emscripten-core:incoming Jun 27, 2019
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
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.

4 participants