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.

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

Compile common libraries to single libs directory #1425

Merged
merged 1 commit into from
Apr 4, 2020

Conversation

themperek
Copy link
Contributor

@themperek themperek commented Feb 19, 2020

  • common libraries are compiled once to /cocotb/libs directory which results in a faster compilation, less space and easier use

  • vpi/vhpi/fli are compiled with _simulator_name sufix (. has special (directory separation) meaning in setuptools) in same directory /cocotb/libs.

  • simulator module is compiled once and located in /cocotb folder. This allows using form cocotb import simulator
    -> simulator module can be added to documentation (need to install/compile) -> future PR.
    -> can remove COCOTB_SIM -> future PR

  • removed from makefiles LD_LIBRARY_PATH everywhere -> less potential collision with user environment settings

Resolves #1181

Checked with ALL supported (Linux) simulators.

@eric-wieser
Copy link
Member

eric-wieser commented Feb 19, 2020

Would it be easier to compile all the libraries (and not just the "common" ones) to a single directory, and call them libcocotbvpi.aldec.so etc? That would reduce the need for the rpath stuff, right?

@themperek
Copy link
Contributor Author

Would it be easier to compile all the libraries to a single directory, and call them libcocotbvpi.aldec.so etc? That would reduce the need for the rpath stuff, right?

Then one needs to change how entry_points work. Possibly can be done but not sure if less ugly.

@eric-wieser
Copy link
Member

Which entry_points are you referring to?

@themperek
Copy link
Contributor Author

themperek commented Feb 19, 2020

Which entry_points are you referring to?

Like here:

GPI_ENTRY_POINT(vpi, register_embed)

/* Use this macro in an implementation layer to define an entry point */
#define GPI_ENTRY_POINT(NAME, func) \
extern "C" { \
void NAME##_entry_point() \
{ \
func(); \
} \
}
#endif /* COCOTB_GPI_PRIV_H_ */

I think I have seen problems here.

@eric-wieser
Copy link
Member

eric-wieser commented Feb 19, 2020

I think we'd only want to change these lines:

std::string full_name = "lib" + *iter + DOT_LIB_EXT;
const char *now_loading = (full_name).c_str();
lib_handle = utils_dyn_open(now_loading);
if (!lib_handle) {
printf("cocotb: Error loading shared library %s\n", now_loading);
exit(1);
}
std::string sym = (*iter) + "_entry_point";
void *entry_point = utils_dyn_sym(lib_handle, sym.c_str());

We use the name in GPIEXTRA twice - once to pick the dll, and another to pick the function name. We could strip the .aldec from the string before doing the second step, and then I think things would work.

@themperek
Copy link
Contributor Author

We use the name in GPIEXTRA twice - once to pick the dll, and another to pick the function name. We could strip the .aldec from the string before doing the second step, and then I think things would work.

Looks like you are right.

@themperek
Copy link
Contributor Author

themperek commented Feb 19, 2020

We use the name in GPIEXTRA twice - once to pick the dll, and another to pick the function name. We could strip the .aldec from the string before doing the second step, and then I think things would work.

I did not write C/C++ code for 10 years. If you give me snippet I can try other way need to read a bit to remember myself how this was done ;-)

@marlonjames
Copy link
Contributor

We could also change GPI_EXTRA format to provide the dll name and entry point name together, rather than enforce libXXX.dll, XXX_entry_point(). This could be useful for cocotb extensions that want to load code into GPI at startup.

@themperek
Copy link
Contributor Author

Could follow the same convencion like some tools loading VPI: libX:entry_pointX,libY:entry_pointY ?

@eric-wieser
Copy link
Member

Sounds like a good idea to me

@eric-wieser
Copy link
Member

eric-wieser commented Feb 22, 2020

Re splitting - one way to do it would be something like this:

std::string arg = "mylib:my_entry_point";

std::string lib_name;
std::string func_name;
auto it = std::find(arg.begin(), arg.end(), ':');
if (it == arg.end()) {
    // no colon in the string
    lib_name= arg;
    func_name = "whatever our default was";
}
else {
    lib_name = std::string(arg.begin(), it);
    func_name = std::string(it+1, arg.end());
}

@themperek themperek force-pushed the try_single_libs branch 3 times, most recently from 732d08d to 0a7901c Compare March 19, 2020 12:27
cocotb/_build_libs.py Outdated Show resolved Hide resolved
@themperek themperek force-pushed the try_single_libs branch 7 times, most recently from 5e9f62f to 61e94fc Compare March 21, 2020 15:29
@themperek themperek marked this pull request as ready for review March 21, 2020 20:46
@themperek themperek removed the status:needs-proprietary-testing Help needed testing on a proprietary tool label Mar 22, 2020
@eric-wieser eric-wieser added this to the 1.4 milestone Mar 22, 2020
@@ -305,6 +330,8 @@ def get_ext():

logger.info("Compiling interface libraries for cocotb ...")
Copy link
Member

Choose a reason for hiding this comment

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

For a future PR: These log lines aren't accurate, all you're doing is producing a list of libraries to be compiled.

cocotb/_build_libs.py Outdated Show resolved Hide resolved
cocotb/_build_libs.py Outdated Show resolved Hide resolved
cocotb/_build_libs.py Outdated Show resolved Hide resolved
cocotb/_build_libs.py Outdated Show resolved Hide resolved
cocotb/_build_libs.py Outdated Show resolved Hide resolved
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Looks good now, thanks for all the work here.

@themperek
Copy link
Contributor Author

Thank you @eric-wieser for all help.

Copy link
Member

@imphil imphil left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! Thanks a lot for pushing this through, it's greatly appreciated.

@eric-wieser
Copy link
Member

Conflicts are with the change to C++, should be easy to resolve.

Remove LD_LIBRARY_PATH from Makfiles

Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:cleanup cleanup or refactoring on code, documentation, or other areas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Library naming and structuring discussion
4 participants