Segmentation fault

OpenKIM
#1

Hi,

I just wanted to update the IMD KIM interface to the latest API (1.2.2) version
but I got a segmentation fault in the process.

This is the valgrind output:

==8622== Invalid read of size 1
==8622== at 0x4C2D144: strcmp (mc_replace_strmem.c:725)
==8622== by 0x548DE3B: KIMBaseElement::equiv(KIM_IOline&, bool)(KIM_API.cpp:529)
==8622== by 0x548FD52: KIM_API_model::is_it_match(KIM_API_model&, KIM_IOline*, int, bool, bool) (KIM_API.cpp:1118)
==8622== by 0x5497C94: KIM_API_model::is_it_match(KIM_API_model&, KIM_API_model&) (KIM_API.cpp:1195)
==8622== by 0x5499E89: KIM_API_model::string_init(char const*, char const*) (KIM_API.cpp:1624)
==8622== by 0x549B6D3: KIM_API_string_init (KIM_API_C.cpp:71)
==8622== by 0x416D1B: init_kim_info (imd_forces_kim.c:100)
==8622== by 0x40A8D6: setup_potentials (imd_potential.c:107)
==8622== by 0x416801: main (imd.c:90)
==8622== Address 0x0 is not stack'd, malloc'd or (recently) free'd

The relevant code from the imd_forces_kim is the following:

void init_kim_info()
{
  char *test_descriptor_string = NULL;
  void *pkim;
  int kimerror;
  real rc = 0.0;

  /* initialize the variables in the kim struct */
  init_kim_object(&kim);

  /* set up a bogus kim descriptor file */
  write_descriptor(&test_descriptor_string);

  /* get KIM API object representing the KIM Model only */
  kimerror = KIM_API_string_init(&pkim, test_descriptor_string, kim_model_name);

I have verified that both test_descriptor_string and kim_model_name are properly initialized.
With an earlier version of the API I did not have any problems with this code.

I currently do not have the time to debug the entire API, maybe someone can point me into
the right direction.

Best regards,
Daniel

#2

Hi Daniel,

Sorry for the trouble. I'll be happy to look into the problem. Can you send me the particular values you are using for test_descriptor_string and kim_model_name? Having those strings would be helpful.

Thanks,

Ryan

#3

Hello Ryan,

I've attached the values of both variables in a file.

Best regards,
Daniel

test_strings (1.29 KB)

#4

Hi Daniel,

OK. I've tracked it down. There were a number of problems with error handling in that routine (KIM_API_string_init()). I've fixed them now. This bug, along with a couple of other recent fixes are enough for me to issue a new release. So, I have now made available openkim-api-v1.2.3.

A second issue that you will need to address is that we have changed some of the key words in the kim descriptor files. We've gone away from things like `real*8' for alternatives like `double'. So, in particular, for your test_descriptor_string you will need to change two occurrences of `real*8' to `double'. Otherwise, you should be good.

Please try this new release and let me know if you have any further trouble.

Cheers,

Ryan Elliott

#5

Hello Ryan,

with the latest version I no longer get a segmentation fault but the
expected KIM error. I will get back to you if there are further problems
besides the real*8 to double transition.

Now I was wondering if it was possible to get some kind of version
information from the API, e.g. with a version macro.
So I could check if the version of the API is supported by
my test and abort otherwise.

Best regards,
Daniel

#6

Hello Ryan,

with the latest version I no longer get a segmentation fault but the
expected KIM error. I will get back to you if there are further problems
besides the real*8 to double transition.

Good. Let us know if any further trouble occurs.

Now I was wondering if it was possible to get some kind of version
information from the API, e.g. with a version macro.
So I could check if the version of the API is supported by
my test and abort otherwise.

Hi Daniel,

This is something I've been thinking about. There is one challenge that I'm not yet sure exactly how to handle. That is the scenario where users want to work directly with the git repository version of the package.

I don't like the idea of having the version number explicitly listed in the files in the git repo because as development occurs between releases the value becomes misleading...

Maybe there is a good way of indicating that the version is a "development" version...

If anyone has an idea of what is a good way to handle this, I'm open to suggestions.

Thanks,

Ryan

#7

Hello Ryan,

with the latest version I no longer get a segmentation fault but the
expected KIM error. I will get back to you if there are further problems
besides the real*8 to double transition.

Good. Let us know if any further trouble occurs.

Now I was wondering if it was possible to get some kind of version
information from the API, e.g. with a version macro.
So I could check if the version of the API is supported by
my test and abort otherwise.

Hi Daniel,

This is something I've been thinking about. There is one challenge that
I'm not yet sure exactly how to handle. That is the scenario where
users want to work directly with the git repository version of the package.

I don't like the idea of having the version number explicitly listed in
the files in the git repo because as development occurs between releases
the value becomes misleading...

Maybe there is a good way of indicating that the version is a
"development" version...

If anyone has an idea of what is a good way to handle this, I'm open to
suggestions.

Perhaps make a version number unrelated to the version of the KIM API. Increase whenever an incompatible change is introduced. So hopefully most of the time this constant would stay the same, but when something like the real*8 to double transition occurs you increase it as soon as the change lands in git. Call it the version of the API not the version of the implementation (which would be something like 1.2.3 at the moment).

Another idea is to split that into major and minor version, where you only increase major when models/tests need code changes (incompatible API change) and minor when adding a new feature that a model/test would depend on (backwards-compatible API change).

Perhaps it would be good to make an official policy for API and ABI stability. How to solve the problem also depends on how that policy looks, I guess.

#8

I would support the idea of having something like an API/ABI level.
This could be implemented rather quickly and is independent of any
major/minor release.

As Tobias already mentioned a stable API/ABI will be necessary for
a widespread usage of the openkim project. Otherwise it would be too
much trouble keeping up with the changes.

Best regards,
Daniel

#9

Daniel and Tobias,

OK. I agree we need versioning information.

I'll start working on setting up versioning...

Ryan

#10

OK. I wanted to update you on this...

I've looked into the issue in detail. We have a tentative plan. We agree that a well defined version policy will be very helpful.

With that in mind, we are considering the adoption of semver-2.0.0
(http://semver.org).

There will be associated api calls to access version information at runtime and #defines to access version information at compile time.

However, at present we feel we are not ready to commit to this scheme. There are many inconsistencies and issues with the current api code. If we adopt semver-2.0.0 right now it is likely the MAJOR version number would quickly grow. We would like to avoid this. We are currently working on a clean redefinition/implementation of the api that we expect to release as v2.0.0 sometime in the first half of 2014. At present our plan is to adopt semver-2.0.0 at that time.

Cheers,

Ryan