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

(272) Added the wide string entry message functions #376

Closed
wants to merge 23 commits into from

Conversation

rmknan
Copy link
Contributor

@rmknan rmknan commented Sep 28, 2023

Added the wide string entry message functions. Changed entry.c, entry.h, entry.cpp and yml files

@rmknan
Copy link
Contributor Author

rmknan commented Sep 28, 2023

#272

@goatshriek goatshriek added the enhancement new features or improvements label Sep 28, 2023
@goatshriek goatshriek self-assigned this Sep 28, 2023
@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.02%. Comparing base (c54a498) to head (5861497).
Report is 27 commits behind head on latest.

Additional details and impacted files
@@            Coverage Diff             @@
##           latest     #376      +/-   ##
==========================================
+ Coverage   91.69%   92.02%   +0.33%     
==========================================
  Files          44       44              
  Lines        3743     3761      +18     
  Branches      489      491       +2     
==========================================
+ Hits         3432     3461      +29     
+ Misses        216      202      -14     
- Partials       95       98       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@goatshriek goatshriek left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together! A few minor comments below, mostly around these two points:

The malloc failure test isn't working properly, likely because the failure size is incorrect. Don't forget that wide characters take up more than one byte! EDIT: actually, the errors thrown in failing cases are invalid encoding, the cause of which I'm not sure offhand - you'll have to investigate further.

The test corpora are currently UTF-8 strings. You'll need to make UTF-16 versions of these in a new folder, test/corpora/wstring, and change your commented out tests to use these (and of course uncomment them so that they run).

* cancelled, due to the use of a lock that could be left locked as well as
* memory management functions.
*
* @since release v2.1.0
Copy link
Owner

Choose a reason for hiding this comment

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

change this to the next release, v2.2.0

test/function/entry.cpp Outdated Show resolved Hide resolved
test/function/entry.cpp Outdated Show resolved Hide resolved
test/function/entry.cpp Outdated Show resolved Hide resolved
@goatshriek
Copy link
Owner

You'll also need to add your new function to src/windows/stumpless.def so that the Windows builds include it.

@rmknan
Copy link
Contributor Author

rmknan commented Oct 10, 2023

I changed the stumpless def file, there were some additional text which gave the error. I am not entirely sure about the valgrind test errors.

@goatshriek
Copy link
Owner

The valgrind errors are happening because there is a memory leak in the tests. Be sure that you free the result of stumpless_get_entry_message after you're finished with it, as it is dynamically allocated.

@rmknan
Copy link
Contributor Author

rmknan commented Oct 12, 2023

I will look into it again, the tests and build did pass locally.

@rmknan
Copy link
Contributor Author

rmknan commented Oct 13, 2023

Hello,

When I try to run Valgrind tests, it does not run and says a file is corrupted as it did in the previous issue. Last time I i solved that by cloning the repo again and making the changes in the new repo as valgrind ran without any problems in the new one.

Now, In the current repo I face the same issue and valgrind does not run. Therefore I tried to reclone the repo again into a separate folder and pasted the changes there. It passes the tests initially and everything runs fine but if I try to make any changes (even if it is commenting ) and re run the checks it says

/usr/bin/ld: CMakeFiles/function-test-entry.dir/test/function/entry.cpp.o: in function `(anonymous namespace)::SetMessageWideStrTest_AsciiMessage_Test::TestBody()':
entry.cpp:(.text+0x45dc4): undefined reference to `stumpless_set_entry_message_str_w'
/usr/bin/ld: CMakeFiles/function-test-entry.dir/test/function/entry.cpp.o: in function `(anonymous namespace)::SetMessageWideStrTest_LongAsciiMessage_Test::TestBody()':
entry.cpp:(.text+0x467d4): undefined reference to `stumpless_set_entry_message_str_w'
/usr/bin/ld: CMakeFiles/function-test-entry.dir/test/function/entry.cpp.o: in function `(anonymous namespace)::SetMessageWideStrTest_MallocFailureOnMessage_Test::TestBody()':
entry.cpp:(.text+0x47396): undefined reference to `stumpless_set_entry_message_str_w'
/usr/bin/ld: CMakeFiles/function-test-entry.dir/test/function/entry.cpp.o: in function `(anonymous namespace)::SetMessageWideStrTest_NullEntry_Test::TestBody()':
entry.cpp:(.text+0x47b8b): undefined reference to `stumpless_set_entry_message_str_w'
/usr/bin/ld: CMakeFiles/function-test-entry.dir/test/function/entry.cpp.o: in function `(anonymous namespace)::SetMessageWideStrTest_NullMessage_Test::TestBody()':
entry.cpp:(.text+0x483bc): undefined reference to `stumpless_set_entry_message_str_w'
/usr/bin/ld: CMakeFiles/function-test-entry.dir/test/function/entry.cpp.o:entry.cpp:(.text+0x48e0a): more undefined references to `stumpless_set_entry_message_str_w' follow

The entry message is mentioned in the yml file but even then it says this.

whereas the current repo's valgrind says this

### unhandled dwarf2 abbrev form code 0x25
### unhandled dwarf2 abbrev form code 0x25
### unhandled dwarf2 abbrev form code 0x25
### unhandled dwarf2 abbrev form code 0x23
### unhandled dwarf2 abbrev form code 0x25
### unhandled dwarf2 abbrev form code 0x25
### unhandled dwarf2 abbrev form code 0x25
### unhandled dwarf2 abbrev form code 0x23
==15962== Valgrind: debuginfo reader: ensure_valid failed:
==15962== Valgrind:   during call to ML_(img_get)
==15962== Valgrind:   request for range [1956341786, +4) exceeds
==15962== Valgrind:   valid image size of 2202296 for image:
==15962== Valgrind:   "/home/rmknan/Github/Stump2/build/function-test-buffer"
==15962== 
==15962== Valgrind: debuginfo reader: Possibly corrupted debuginfo file.
==15962== Valgrind: I can't recover.  Giving up.  Sorry.
==15962== 

@goatshriek
Copy link
Owner

It will be some time before I can get to troubleshooting this myself, as there are a few other pieces of the library that I'm spending time on right now. The errors you mentioned above seem like you may need to re-build the test after changing source, try building the function-test-entry target and running again.

@rmknan
Copy link
Contributor Author

rmknan commented Oct 16, 2023

The valgrind errors are happening because there is a memory leak in the tests. Be sure that you free the result of stumpless_get_entry_message after you're finished with it, as it is dynamically allocated.

new_message is the result of get_entry_message. So doing free( ( void * ) new_message ); should solve the valgrind error right?

@goatshriek
Copy link
Owner

Yes, that's likely where the memory leak is coming from.

@rmknan
Copy link
Contributor Author

rmknan commented Oct 16, 2023

I did that but valgrind still says the same

@rmknan
Copy link
Contributor Author

rmknan commented Oct 17, 2023

Is there anything else I need to look at?

@goatshriek
Copy link
Owner

If you feel that you've exhausted what you're able to resolve, you can push your current state and I'll go through and resolve the remaining errors.

Unfortunately, I don't have the time to go through and create another review detailing the issues since doing a local checkout, troubleshooting, and writeup is quite time consuming. If you do find that you need me to complete this change, I'll start with your current state and open a new PR with everything resolved. You'll have the opportunity to provide a final review of that before it's merged.

Note that this will result in you being a co-author (with me as the other) of the final commit instead of the sole author. If this is important to you, you have lots of time to keep working through this. I certainly don't want you to feel like I've stolen credit for your hard work, so I won't move forward with that course of action unless you let me know that's how you want to proceed, this goes stale as defined in the inactivity policy, or we approach the next release deadline, which will probably be in December of this year.

@rmknan
Copy link
Contributor Author

rmknan commented Oct 18, 2023

I understand and thank you for sharing that. I will work on this and try to come up with a solution. I can not think of any reason for the valgrind to fail now so I think I will focus on other issues and take this one slow. If you do get some free time and are able to share any insight please do.

I honestly don't mind being a co author but I would love to still solve this.

@goatshriek goatshriek added the stale inactive issue or pull request label Mar 30, 2024
@goatshriek
Copy link
Owner

Incorporated into #412

@goatshriek goatshriek closed this Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new features or improvements stale inactive issue or pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants