starwing / amoeba

a Cassowary constraint solving algorithm implements in pure C.
MIT License
177 stars 24 forks source link

Read access violation at amoeba.h line 289 #2

Closed hsa-online closed 7 years ago

hsa-online commented 7 years ago

I found another problem about a minute ago.

It's necessary to add to test.c after the code:

        // Ycur = Yprev_row + 10
        pC = am_newconstraint(pSolver, AM_REQUIRED);
        am_addterm(pC, arrY[nCurrentRowFirstPointIndex + nPoint], 1.0);
        am_setrelation(pC, AM_EQUAL);
        am_addterm(pC, arrY[nCurrentRowFirstPointIndex - 1], 1.0);
        am_addconstant(pC, 10.0);
        nResult = am_add(pC);
        assert(nResult == AM_OK);

These lines: // Add these to test.c line 282 pC = am_newconstraint(pSolver, AM_REQUIRED); am_addterm(pC, arrX[nCurrentRowFirstPointIndex + nPoint], 1.0); am_setrelation(pC, AM_GREATEQUAL); am_addconstant(pC, 0.0); nResult = am_add(pC); assert(nResult == AM_OK);

And it causes an access violation at amoeba.h line 289

P.S. I also found some strange situations when X-variables are became negative but still testing these ones.

starwing commented 7 years ago

I have noticed this and fix it (I hope so).

it because I moved the nullptr test into am_remove(), but not implement correctly.

you could try it out with the latest commit.

Sorry for my WIP state :(

hsa-online commented 7 years ago

I just tried last commit, and the problem is still here.

Sorry for my WIP state :(

No problem! "Progress" is always good. :-)

starwing commented 7 years ago

But I add test to line 282, I don't find access violation :-(

hsa-online commented 7 years ago

I prepared smallest possible test showing this problem: test2.txt It crashes in 100% of cases (tested under Windows 7 x64, compiled in both x86 and x64).

Also in x64 there are 4 warnings on compile: amoeba.h(361): warning C4244: '=': conversion from 'int64' to 'int', possible loss of data amoeba.h(363): warning C4244: '+=': conversion from 'int64' to 'int', possible loss of data amoeba.h(366): warning C4244: '=': conversion from 'int64' to 'int', possible loss of data amoeba.h(368): warning C4244: '=': conversion from 'int64' to 'int', possible loss of data

starwing commented 7 years ago

okay, thanks for reporting it, I have found the issue and fix the warnings also, you can try it out :-)

starwing commented 7 years ago

it seems that you are using C++, I'm planing to write a C++11 wrapper for this library, so if you like it, you can raise a new issue to follow the work on C++ wrapper.

hsa-online commented 7 years ago

I tested new version. It doesn't more crash on am_add(...) but it now generates "assertion failed" on am_delsolver() call:

In am_freepool() the line assert(pool->count == 0); causes "assertion failed" as in my case the pool->count equals to 1011

Regarding C++11 wrapper: I don't know still because to use your library I must absolutely be sure that even on bad/incomplete data it never crashes.

So if you plan to make it better I'll continue to test it. :-)

starwing commented 7 years ago

Yes, I will make it better.

Very sorry for the assertion failed/crash for the library. because I can not find good test cases. I will make a travis-cl config and a coverage test to make sure it won't crash on any data.

maybe you can help adding the data case, with travis it might stable than currently.

starwing commented 7 years ago

for this assertion failed: this assertions failed means "you will have memory leak when you delete solver", you may have objects new-ed but not deleted. I will check whether is the internal issue or the mis-usage, and if it's a mis-usage, I will change the interface to avoid it.

starwing commented 7 years ago

I have checked and revert the changes from last commit. I used to add these check before, and removed it after found it will fail because hashtable still hold pool memories. coding in late night is not good :(

I will add travis immediately when I have time. I will fix all crash you find. the amoeba itself not easy crash, most of them are unchecked null pointer API call or un-needed assertion failure. but I will add test to check the coverage of this project when it become stable.

and I must against the desire to change the code before my brain T_T

hsa-online commented 7 years ago

Regarding the assertions you wrote:

this assertions failed means "you will have memory leak when you delete solver", you may have objects new-ed but not deleted

Yes, I understood.

maybe you can help adding the data case, with travis it might stable than currently.

OK. I added support of auto-tests (via the 'Catch').

starwing commented 7 years ago

I have add travis auto test and code coverage test. I will work to coverage 100%, in this way the crash will never occurs. you could use 'passing' commit and treat it stable.

hsa-online commented 7 years ago

Ok, thanks.

starwing commented 7 years ago

@hsa-online any news?

hsa-online commented 7 years ago

@hsa-online any news?

I wrote my own implementation of Cassowary so I stopped to test your library.