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

Fix tests passing when test commands fail #1733

Merged
merged 3 commits into from
Sep 28, 2023

Conversation

brechtvl
Copy link
Contributor

@brechtvl brechtvl commented Sep 28, 2023

Description

37cbab5 accidentally changed tests to accept failing commands by default.

This can cause tests to pass even though the program crashed, when a previous run created the correct output file.

This reveals existing asan and lsan errors. The asan error was fixed. The memory leaks remain ignored, as the exact cause is unclear.

Tests

N/A

Checklist:

  • I have read the contribution guidelines.
  • I have previously submitted a Contributor License Agreement.
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
  • My code follows the prevailing code style of this project.

37cbab5 accidentally changed tests to accept failing commands by
default.

This can cause tests to pass even though the program crashed, when
a previous run created the correct output file.

Signed-off-by: Brecht Van Lommel <brecht@blender.org>
Signed-off-by: Brecht Van Lommel <brecht@blender.org>
@brechtvl
Copy link
Contributor Author

I made it pass tests even when there are memory leaks. Better would be to fix these (if possible), but I don't really have the time for that. There is not a lot to go on.

=================================================================
==17951==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 140 byte(s) in 1 object(s) allocated from:
    #0 0x510ab7 in operator new(unsigned long) /llvm-project-llvmorg-13.0.1/compiler-rt/lib/asan/asan_new_delete.cpp:95:3
    #1 0x7f2828947cd8 in std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator<char> const&) (/lib64/libstdc++.so.6+0xbdcd8)

Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x510d67 in operator new(unsigned long, std::nothrow_t const&) /llvm-project-llvmorg-13.0.1/compiler-rt/lib/asan/asan_new_delete.cpp:101:3
    #1 0x7f28288e6f1d in __cxa_thread_atexit (/lib64/libstdc++.so.6+0x5cf1d)

SUMMARY: AddressSanitizer: 164 byte(s) leaked in 2 allocation(s).

@lgritz
Copy link
Collaborator

lgritz commented Sep 28, 2023

I don't mind sweeping a leak of 164 bytes under the rug for now, but does this change mask any other failures in the sanitizer run, which we would definitely want to catch?

Signed-off-by: Brecht Van Lommel <brecht@blender.org>
@brechtvl
Copy link
Contributor Author

It was masking all memory leaks. I made it more specific now, though it's still pretty broad.

Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

LGTM

Apologies on the "failureok = 1" mistake -- that was me, probably something I had temporarily done while testing, and then forgot to restore.

@lgritz lgritz merged commit 0210c88 into AcademySoftwareFoundation:main Sep 28, 2023
lgritz added a commit that referenced this pull request May 8, 2024
A few months back, PR #1733 seems to have switched the order that
testrender destroys the shading system versus the renderer (services).

This made some subtle bugs that were only symptomatic for GPU renders,
but it's because of the destructor order, where the shadingsystem's
dtr still references the renderer, which cannot be destroyed yet.

The clue is that the SS's constructor takes the RS pointer as an
argument. The RS, then, must have been constructed before the SS, and
therefore we should expect it to be a requirement for the RS to
outlast the lifetime of the SS. (Complex objects should be destroyed
in the opposite order that they were constructed, if they contain
references to each other.)

One code change is needed to avoid the sanitizer errors that the
incorrect change was originally meant to address: clear shaders when
SimpleRayTracer clears.

Signed-off-by: Larry Gritz <lg@larrygritz.com>

---------

Signed-off-by: Larry Gritz <lg@larrygritz.com>
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