Skip to content
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.

Sign up for GitHub

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

Add a check for external definitions that are not thread safe #920

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add a check for external definitions that are not thread safe #920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions src/lexer/token_mapping.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,31 @@ const static std::vector<std::string> extern_definitions = {"acos",
"tanh",
"threshold"};
const static std::vector<std::string> need_nt = {"at_time"};
const static std::vector<std::string> not_thread_safe = {"force",
"deflate",
"expfit",
"derivs",
"spline",
"exprand",
"gauss",
"normrand",
"poisrand",
"poisson",
"setseed",
"scop_random",
"boundary",
"romberg",
"invert",
"stepforce",
"schedule",
"set_seed",
"nrn_random_play"};

bool is_external_definitions(const std::string& token) {
return std::find(extern_definitions.cbegin(), extern_definitions.cend(), token) !=
extern_definitions.cend();
}


/**
* Checks if \c token is one of the functions coming from NEURON/CoreNEURON and needs
Expand All @@ -257,6 +282,12 @@ bool needs_neuron_thread_first_arg(const std::string& token) {
}


bool is_not_thread_safe(const std::string& token) {
return std::find(not_thread_safe.cbegin(), not_thread_safe.cend(), token) !=
not_thread_safe.cend();
}


/**
* Variables from NEURON that are directly used in NMODL
*
Expand Down
2 changes: 2 additions & 0 deletions src/lexer/token_mapping.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ std::vector<std::string> get_external_functions();

namespace details {

bool is_external_definitions(const std::string& token);
bool needs_neuron_thread_first_arg(const std::string& token);
bool is_not_thread_safe(const std::string& token);

} // namespace details

Expand Down
13 changes: 13 additions & 0 deletions src/visitors/semantic_analysis_visitor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "ast/program.hpp"
#include "ast/suffix.hpp"
#include "ast/table_statement.hpp"
#include "lexer/token_mapping.hpp"
#include "symtab/symbol_properties.hpp"
#include "utils/logger.hpp"
#include "visitors/visitor_utils.hpp"
Expand Down Expand Up @@ -101,5 +102,17 @@ void SemanticAnalysisVisitor::visit_destructor_block(const ast::DestructorBlock&
/// -->
}

void SemanticAnalysisVisitor::visit_function_call(const ast::FunctionCall& node) {
/// <-- This code is for check 5
const auto& name = node.get_node_name();
if (details::is_external_definitions(name) && details::is_not_thread_safe(name)) {
logger->critical(
"SemanticAnalysisVisitor :: '{}' is not thread safe and incompatible with CoreNEURON",
Copy link
Contributor

@iomaganaris iomaganaris Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm wondering whether instead of this message we should print that the problem is that these functions are not implemented in CoreNEURON (which will need to change if at some point we change the nmodl translator of NEURON with this NMODL).
@pramodk what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree - the thread safety aspect is « implementation » aspect. I believe it’s not really about the method themselves.

Ioannis - as part of your work, we need to find out what needs to be done with these (legacy) functions.

And if you want to move this PR forward, throwing error as not implemented is better than saying not thread safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alkino @iomaganaris : something else relevant comes to my mind - if you look at the CI then it fails for netstim.mod for the usage of set_seed and exprand.

There are a few things to remember for thread safety: there are only certain blocks of the MOD file are called from the parallel region and they are: INITIAL, BREAKPOINT, DERIVATIVE, KINETIC, NET_RECEIVE (and something else?). If we are using non-thread-safe functions from these blocks (directly or indirectly) then only it's problem. Given this, I think we can bit improve this pass when to show a warning. For example:

  • set_seed(): this function is called from PROCEDURE seed(x) but this procedure is not called from INITIAL, BREAKPOINT, DERIVATIVE, KINETIC or NET_RECEIVE. This means this procedure is most likely called from the interpreter (hoc/python) side in a serial execution. Hence this usage is not really problematic.

  • exprand() is called indirectly from INITIAL/NET_RECEIVE->invl->erand so this usage is "potentially" problematic. But it's difficult to say for sure because exprand is preceded by the usage of VERBATIM block and hence this could be conditional usage for NEURON with #if !NRNBBCORE etc.

What I am trying to say is that it might be possible to do this check bit better? I haven't thought about the implementation aspect. If it's difficult, we can leave this. But I thought it's worth mentioning.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alkino : I am reviving this after 2 years!

As NEURON code generation is ongoing, I think this PR is relevant and good to have! If this PR is blocked for my earlier comment, especially set_seed and exprand, then I would say we can exclude these two functions and get the rest merged.

What do you think?

name);
check_fail = true;
}
/// -->
}

} // namespace visitor
} // namespace nmodl
5 changes: 5 additions & 0 deletions src/visitors/semantic_analysis_visitor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
* 2. Check that destructor blocks are only inside mod file that are point_process.
* 3. A TABLE statement in functions cannot have name list, and should have one in procedures.
* 4. Check if ion variables from a `USEION` statement are not declared in `CONSTANT` block.
* 5. Check that an external definition is allowed by CoreNeuron
*/
#include "ast/ast.hpp"
#include "visitors/ast_visitor.hpp"
Expand Down Expand Up @@ -60,6 +61,10 @@ class SemanticAnalysisVisitor: public ConstAstVisitor {
/// Visit destructor and check that the file is of type POINT_PROCESS or ARTIFICIAL_CELL
void visit_destructor_block(const ast::DestructorBlock& node) override;

/// Visit function call and check if the function is not thread safe and so not
/// compatible with CoreNeuron
void visit_function_call(const ast::FunctionCall& node) override;

public:
SemanticAnalysisVisitor() = default;

Expand Down