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

Updated comments to Doxygen format #6

Merged
merged 5 commits into from
Nov 8, 2021
Merged

Conversation

redcarrera
Copy link
Contributor

Includes minor code format changes

Chambers added 3 commits November 2, 2021 16:57
Includes minor code format changes
Other minir format changes

Signed-off-by: Chambers <1594260127121005@mil>
Signed-off-by: Chambers <1594260127121005@mil>
Chambers added 2 commits November 4, 2021 15:56
Signed-off-by: Chambers <1594260127121005@mil>
Signed-off-by: Chambers <1594260127121005@mil>
@brlcad
Copy link
Member

brlcad commented Nov 8, 2021

@redcarrera merging them, but curious why you're converting many of the comments to doxygen. The creo plugin implementation isn't public API, so we're not going to be generating (or at least shouldn't be generating) doxygen API output for it. Are we generating doxygen output for it?

Doxygen could be used to document the plugin's implementation, so I assume that's what you were going for and merge accordingly. I'll note, though, that some of the inline comments that were converted don't apply as you maybe intended. That is, the "labels" often found on variable declarations like:

/** these are important*/
int a;
int b;
int c;

/** more super-important variables */
char *d;
char *e;

If you make "these are important" a doxygen comment, it only applies to the 'a' variable where the intent is to document a, b, and c. That's also a case that really neither should be made into a doxygen comment regardless as they're not documenting API.

Still, like I said, merging because this set of changes shouldn't cause a problem regardless. Thanks for cleaning up the plugin!

@brlcad brlcad merged commit a83a34b into BRL-CAD:main Nov 8, 2021
starseeker added a commit that referenced this pull request Dec 8, 2021
We don't want this to accidentally activate if -lasan is no longer a
requirement someday - BRL-CAD's code isn't ready for it yet.  In
particular, the LeakSanitizer trips up on the rt_free_rti/rt_clean
tangle as well as _db_walk_subtree.  The former may need elimination of
the rt_uniresource global to properly clean up, and the latter is deep
in the raytracing core and will have to be approached very carefully.

For convenience, here are the backtraces:

889: Test command: build/bin/gqa "-P" "1" "-Ao" "-p" "ovlp_" "brlcad/src/gtools/tests/../../libged/tests/ged_tests.g" "ovlp"
889: Test timeout computed to be: 1500
889: Trying initial grid spacing: 50 mm
889: Using grid spacing lower limit: 0.005 mm
889: Plotting overlaps to ovlp_overlaps.plot3
889: Processing with grid spacing 50 mm 44 x 48 x 39
889: Summary (50mm grid spacing):
889: list Overlaps:
889: /ovlp/r1 /ovlp/r2 count:2277 dist:1780.3mm @ (-248 350 -903.325)
889:
889: =================================================================
889: ==672620==ERROR: LeakSanitizer: detected memory leaks
889:
889: Direct leak of 524288 byte(s) in 1024 object(s) allocated from:
889:     #0 0x498087 in posix_memalign (build/bin/gqa+0x498087)
889:     #1 0x7f9d3ca476c3 in alloc brlcad/src/libbu/malloc.c:137:10
889:     #2 0x7f9d3ca478f6 in bu_calloc brlcad/src/libbu/malloc.c:174:12
889:     #3 0x7f9d3ca9b25e in bu_ptbl_init brlcad/src/libbu/ptbl.c:45:26
889:     #4 0x7f9d40d38be4 in rt_init_resource brlcad/src/librt/prep.c:855:2
889:     #5 0x7f9d40d3ba65 in rt_clean_resource brlcad/src/librt/prep.c:1063:5
889:     #6 0x7f9d40d3239a in rt_clean brlcad/src/librt/prep.c:1210:6
889:     #7 0x7f9d40d30f80 in rt_free_rti brlcad/src/librt/prep.c:170:5
889:     #8 0x7f9d380c4573  (<unknown module>)
889:     #9 0x7f9d424f2526 in ged_exec brlcad/src/libged/exec.cpp:83:16
889:     #10 0x7f9d424fd51b in ged_gqa brlcad/src/libged/exec_mapping.cpp:177:1
889:     #11 0x4c8b17 in main brlcad/src/gtools/gqa.c:105:11
889:     #12 0x7f9d3c6be0b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
889:
889: Direct leak of 524288 byte(s) in 1024 object(s) allocated from:
889:     #0 0x498087 in posix_memalign (build/bin/gqa+0x498087)
889:     #1 0x7f9d3ca476c3 in alloc brlcad/src/libbu/malloc.c:137:10
889:     #2 0x7f9d3ca478f6 in bu_calloc brlcad/src/libbu/malloc.c:174:12
889:     #3 0x7f9d3ca9b25e in bu_ptbl_init brlcad/src/libbu/ptbl.c:45:26
889:     #4 0x7f9d40d38c65 in rt_init_resource brlcad/src/librt/prep.c:858:2
889:     #5 0x7f9d380c0db2  (<unknown module>)
889:     #6 0x7f9d424f2526 in ged_exec brlcad/src/libged/exec.cpp:83:16
889:     #7 0x7f9d424fd51b in ged_gqa brlcad/src/libged/exec_mapping.cpp:177:1
889:     #8 0x4c8b17 in main brlcad/src/gtools/gqa.c:105:11
889:     #9 0x7f9d3c6be0b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
889:
889: Direct leak of 64 byte(s) in 2 object(s) allocated from:
889:     #0 0x498087 in posix_memalign (build/bin/gqa+0x498087)
889:     #1 0x7f9d3ca476c3 in alloc brlcad/src/libbu/malloc.c:137:10
889:     #2 0x7f9d3ca478f6 in bu_calloc brlcad/src/libbu/malloc.c:174:12
889:     #3 0x7f9d40ce3610 in _db_walk_subtree brlcad/src/librt/db_tree.c:1914:6
889:     #4 0x7f9d40cdd664 in _db_walk_dispatcher brlcad/src/librt/db_tree.c:1992:2
889:     #5 0x7f9d3ca795da in bu_parallel brlcad/src/libbu/parallel.c:574:2
889:     #6 0x7f9d40cdc2a2 in db_walk_tree brlcad/src/librt/db_tree.c:2199:5
889:     #7 0x7f9d41893a86 in rt_gettrees_muves brlcad/src/librt/tree.c:779:6
889:     #8 0x7f9d4189bb3f in rt_gettrees_and_attrs brlcad/src/librt/tree.c:883:12
889:     #9 0x7f9d4189bcba in rt_gettree brlcad/src/librt/tree.c:896:11
889:     #10 0x7f9d380c0e88  (<unknown module>)
889:     #11 0x7f9d424f2526 in ged_exec brlcad/src/libged/exec.cpp:83:16
889:     #12 0x7f9d424fd51b in ged_gqa brlcad/src/libged/exec_mapping.cpp:177:1
889:     #13 0x4c8b17 in main brlcad/src/gtools/gqa.c:105:11
889:     #14 0x7f9d3c6be0b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
889:
889: Direct leak of 64 byte(s) in 2 object(s) allocated from:
889:     #0 0x498087 in posix_memalign (build/bin/gqa+0x498087)
889:     #1 0x7f9d3ca476c3 in alloc brlcad/src/libbu/malloc.c:137:10
889:     #2 0x7f9d3ca478f6 in bu_calloc brlcad/src/libbu/malloc.c:174:12
889:     #3 0x7f9d40c2a4bc in rt_comb_import5 brlcad/src/librt/comb/comb.c:518:6
889:     #4 0x7f9d40c79613 in rt_db_external5_to_internal5 brlcad/src/librt/db5_io.c:958:8
889:     #5 0x7f9d40c79e9f in rt_db_get_internal5 brlcad/src/librt/db5_io.c:998:11
889:     #6 0x7f9d40ce6b76 in rt_db_get_internal brlcad/src/librt/dir.c:131:9
889:     #7 0x7f9d40cd31b4 in db_recurse brlcad/src/librt/db_tree.c:1016:6
889:     #8 0x7f9d40ce34ee in _db_walk_subtree brlcad/src/librt/db_tree.c:1898:16
889:     #9 0x7f9d40cdd664 in _db_walk_dispatcher brlcad/src/librt/db_tree.c:1992:2
889:     #10 0x7f9d3ca795da in bu_parallel brlcad/src/libbu/parallel.c:574:2
889:     #11 0x7f9d40cdc2a2 in db_walk_tree brlcad/src/librt/db_tree.c:2199:5
889:     #12 0x7f9d41893a86 in rt_gettrees_muves brlcad/src/librt/tree.c:779:6
889:     #13 0x7f9d4189bb3f in rt_gettrees_and_attrs brlcad/src/librt/tree.c:883:12
889:     #14 0x7f9d4189bcba in rt_gettree brlcad/src/librt/tree.c:896:11
889:     #15 0x7f9d380c0e88  (<unknown module>)
889:     #16 0x7f9d424f2526 in ged_exec brlcad/src/libged/exec.cpp:83:16
889:     #17 0x7f9d424fd51b in ged_gqa brlcad/src/libged/exec_mapping.cpp:177:1
889:     #18 0x4c8b17 in main brlcad/src/gtools/gqa.c:105:11
889:     #19 0x7f9d3c6be0b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
889:
889: SUMMARY: AddressSanitizer: 1048704 byte(s) leaked in 2052 allocation(s).
starseeker added a commit that referenced this pull request Dec 8, 2021
Oof.  Writing down the details on this one, as I doubt I'll remember...

The start was turning on both the AddressSanitizer and Qt.  When doing
so, most programs suddenly began reporting memory leaks.  However, the
report was rather cryptic, being four entries similar to this one:

==1262202==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 21 byte(s) in 1 object(s) allocated from:
    #0 0x498087 in posix_memalign (build/src/libdm/tests/dm_test+0x498087)
    #1 0x7f4e90e53675 in alloc brlcad/src/libbu/malloc.c:129:10
    #2 0x7f4e90e53400 in bu_malloc brlcad/src/libbu/malloc.c:167:12
    #3 0x7f4e90ee1c02 in bu_strdupm brlcad/src/libbu/str.c:165:17
    #4 0x7f4e87dcd3e9  (<unknown module>)
    #5 0x7f4e87dcdea5  (<unknown module>)
    #6 0x7f4e9650eb89  (/lib64/ld-linux-x86-64.so.2+0x11b89)

I wasn't immediately sure if the unknown module error was related to the
plugin loading, and spent a lot of time trying to find a string memory
issue in dm_init.cpp.  After that proved fruitless, other than to
confirm that the error disappeared if i removed the bu_dlclose-ing of
the handles saved at initialization, I began to look for ways to decode
the "unknown module" entries.  That led to the following issue:
google/sanitizers#89 which describes the
problem ASan has with dynamic libraries.  Comments indicated that it
would see real issues, but won't report which dynamic library they come
from (and there are no plans to fix this anytime soon... grr...).  In
fairness, valgrind can see the error too but also has the same reporting
problem; it appears to be ubiquitous.

Fortunately, we have an alternative due to the way our plugin system and
test apps work - we can simply add and remove .so files to the directory
and see how the error reporting changes to zero in on which file(s) are
triggering the problem.  Doing so quickly made apparent what should have
been obvious in retrospect - two of the errors each were coming from the
Qt and swrast plugins, which had been off in earlier testing.

Since there was no backtrace beyond the bu_strdupm call itself, and
there were two errors per file, the suspect was the bu_strdup calls
initializing the "char *" names for the fb structures.  The C files use
static strings (which is why the non-Qt plugins didn't show the issue),
but C++ doesn't tolerate the type mismatch.  The original hack
workaround was just to bu_strdup and create a (char *) string, but as
the leak detectors correctly note this also means there's no way to
clean up the allocated memory.

As far as I can tell there is no reason for these strings to be editable
(char *) strings - the dm container's equivalents are static.  This
commit removes any logic assuming if_name is dynamic, and also removes
the bu_strdup hack from Qt and swrast.
zhuodannychen added a commit to zhuodannychen/brlcad that referenced this pull request May 9, 2023
fixed a bunch of bugs, still need to integrate options with renderer
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.

None yet

3 participants