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

refs #342 - do not load included files twice in CLI application / added DUI::removeComment #340

Merged
merged 2 commits into from
Mar 8, 2024

Conversation

firewave
Copy link
Collaborator

@firewave firewave commented Feb 16, 2024

The CLI application was pre-loading the included files just to remove the comments. Adding a DUI option for that makes it necessary. This makes profiling with the CLI more useful and it also fixes a potential memory leak in the CLI when an empty (or currently also non-existent) include was specified.

@firewave
Copy link
Collaborator Author

Still a draft as I want to research this a bit more.

Sample which produces a leak (still looking at getting one with valid code):

#include  <//.>3=i`/\   >
 88longm a
==21281==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 72 byte(s) in 1 object(s) allocated from:
    #0 0x7fdc95ee2002 in operator new(unsigned long) /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_new_delete.cpp:95
    #1 0x564f6098d585 in simplecpp::load(simplecpp::TokenList const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&, simplecpp::DUI const&, std::__cxx11::list<simplecpp::Output, std::allocator<simplecpp::Output> >*) /home/user/CLionProjects/simplecpp-rider/simplecpp.cpp:3180

SUMMARY: AddressSanitizer: 72 byte(s) leaked in 1 allocation(s).

@firewave firewave changed the title do not load incliuded files twice in CLI application / added DUI::removeComment do not load included files twice in CLI application / added DUI::removeComment Feb 16, 2024
@firewave
Copy link
Collaborator Author

firewave commented Feb 17, 2024

Here is a very sane looking example:

#include</\\>
#include</\\>
==58215==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 144 byte(s) in 2 object(s) allocated from:
    #0 0x7ff948ae2002 in operator new(unsigned long) /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_new_delete>
    #1 0x56404715371a in simplecpp::load(simplecpp::TokenList const&, std::vector<std::__cxx11::basic_string<>

Direct leak of 72 byte(s) in 1 object(s) allocated from:
    #0 0x7ff948ae2002 in operator new(unsigned long) /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_new_delete>
    #1 0x564047162412 in simplecpp::preprocess(simplecpp::TokenList&, simplecpp::TokenList const&, std::vecto>

SUMMARY: AddressSanitizer: 216 byte(s) leaked in 3 allocation(s).

The second memory leak from that example is fixed by #339.

We should also add a testcase in Cppcheck when this hits downstream.

@firewave firewave marked this pull request as ready for review February 17, 2024 00:53
@firewave firewave marked this pull request as draft February 17, 2024 00:54
@firewave firewave changed the title do not load included files twice in CLI application / added DUI::removeComment refs #342 - do not load included files twice in CLI application / added DUI::removeComment Feb 22, 2024
@firewave
Copy link
Collaborator Author

This fixes the first memory leak from #342.

@firewave firewave marked this pull request as ready for review February 22, 2024 18:46
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

lgtm.. do you feel that this is finished?

@firewave
Copy link
Collaborator Author

lgtm.. do you feel that this is finished?

Yes - on the CLI side this is finished. Otherwise I would have left it a draft :)

@danmar danmar merged commit ad9b49d into danmar:master Mar 8, 2024
14 checks passed
@firewave firewave deleted the comments branch March 8, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants