Remove exit() from emulator

[jslee02]

This PR makes easier to debugging for the unit tests as we can use breakpoints without modifying the test code.

[prasbg]

(whoops, wrong account 🤦‍♂️)

[psigen]

This seems to make sense.

At some point can you explain how the Emulator system works now w.r.t. the other tests? I don't remember reviewing that change.

[jslee02]

Thanks for the review! I'm currently fixing build error in Xenial.

At some point can you explain how the Emulator system works now w.r.t. the other tests? I don't remember reviewing that change.

In short, the Emulator is a helper class to help passing arguments to the Chimera ClangTool in a easier way. It manages the resource paths so that we can pass relative paths, which is much shorter than the full path. I'm open to improve further the Emulator or replace it with a better way.

[jslee02]

@psigen Have you seen this errors before? 🤔

: for the -p option: may only occur zero or one times!
: for the -b option: may only occur zero or one times!
: for the -c option: may only occur zero or one times!

This only occurs on Xenial.

[psigen]

That would seem to indicate that clang-tool is getting multiple copies of those flags. Can you inspect the argv list around where that error is occurring (just before the call to clang itself that starts the tool) and see if duplicate entries have been inserted somehow?

If that's not the case, it's also possible that internal to clang there is a singleton storing argument data that is not resetting between tests, this was my concern when initially implementing that led me to split the tests out of caution-- I didn't know what internal state clang might have.

[jslee02]

Can you inspect the argv list around where that error is occurring (just before the call to clang itself) and see if duplicate entries have been inserted somehow?

https://dev.azure.com/personalrobotics/chimera/_build/results?buildId=463&view=logs&j=ab86883e-e9b1-5ace-f4e3-8fcf9953986c&t=ef4a0dfa-4f6e-589b-e06d-c21cfa2b3059&l=833
There is no duplicates.

If that's not the case, it's also possible that internal to clang there is a singleton storing argument data that is not resetting between tests, this was my concern when initially implementing that led me to split the tests out of caution-- I didn't know what internal state clang might have.

It seems this is true.

[psigen]

Well, maybe we can try to look for some clang functionality to forcibly reset the internal state? I will look through the API.

[jslee02]

At worst, we could use the old behavior (using exit or something else) as workaround for Xenial (probably for LLVM <= 3.9)

[psigen]

Also is it safe to do this with our singleton usage?

chimera/src/chimera.cpp

Line 100 in d3ff5e8

chimera::Configuration::GetInstance().LoadFile(ConfigFilename);

[psigen]

Can you tell if the errors are originating around this line?

chimera/src/chimera.cpp

Line 95 in d3ff5e8

CommonOptionsParser OptionsParser(argc, argv, ChimeraCategory);

[jslee02]

Also is it safe to do this with our singleton usage?

It's not. How do you think we add Clear() to the configuration to clear the internal state before using for a new test?

[psigen]

It would not be easy to do safely. The best way to handle this would be to figure out how to not use the singleton in the first place, which would involve figuring out how to pass values through ClangTool's FrontEnd Factory interface 😓

Is it at all possible to leave the exit functionality but figure out how to make breakpoints work with it? I'm not sure I understood fully why it didn't work before.

[jslee02]

It would not be easy to do safely. The best way to handle this would be to figure out how to not use the singleton in the first place, which would involve figuring out how to pass values through ClangTool's FrontEnd Factory interface

One possible way to create an configuration manager, which is a singleton, and we just tell the manager which configuration to use. The API would be like:

Config config;
// setting the config

ConfigManager::GetInstance().setConfig(config);

// in the custom ClangTool
auto& config = ConfigManager::GetInstance().getConfig();

Is it at all possible to leave the exit functionality but figure out how to make breakpoints work with it? I'm not sure I understood fully why it didn't work before.

Let me double check.

[jslee02]

Is it at all possible to leave the exit functionality but figure out how to make breakpoints work with it? I'm not sure I understood fully why it didn't work before.

Well, it turns out that this is because EXPECT_EXIT executes the expression in a separate thread, and I'm just not familiar with debugging multiple threads with Qt Creator.

[psigen]

Given that we don't quite understand what clang is doing internally here, my preference is to continue to use EXIT and just see if we can figure out how to do threaded debugging: it should be possible.

I would rather not use a singleton instance manager as this is adding complexity into the main code path for the sake of only the test harness.

We can see if there's a way to actually pass this as an argument into the visitor via some sort of userdata. It's been a while since we looked, it's possible there's a mechanism we missed before.

[psigen]

Here is an approach for fixing the singleton issue:
#281

[jslee02]

The issue still exists even with #281.

[psigen]

Unfortunately, I think that might mean clang is caching the argument parsing somehow.
I am suspicious of the OptionsParser, we should see if the output it provides after parsing looks correct.