Allocator: Add macro for simple allocation name
Needs ReviewPublic
Actions

Authored by Aaron Carlisle (Blendify) on Dec 26 2021, 1:17 AM.

Details

Reviewers

Campbell Barton (campbellbarton)
Brecht Van Lommel (brecht)
Jacques Lucke (JacquesLucke)

Summary

Recent changes to our node code makes many function names the same between node files, for example node_init().
As a result there are many different allocations in different files but same function names.

Thus using just __func__ as the allocation name is not useful for debuging.

As a solution a macro is added to give the file name and line number.

I left this as an if toggle with the other option of __func__ because I was told some developers prefer the function name.

Diff Detail

Repository

rB Blender

Branch

guardealloc_mem_at (branched from master)

Build Status

Buildable 20048
Build 20048: arc lint + arc unit

Event Timeline

Aaron Carlisle (Blendify) requested review of this revision.Dec 26 2021, 1:17 AM

Aaron Carlisle (Blendify) created this revision.

Harbormaster completed remote builds in B19630: Diff 46387.Dec 26 2021, 1:17 AM

Aaron Carlisle (Blendify) added reviewers: Campbell Barton (campbellbarton), Brecht Van Lommel (brecht), Jacques Lucke (JacquesLucke).Dec 26 2021, 1:19 AM

Fix missed rename

Harbormaster completed remote builds in B19631: Diff 46388.Dec 26 2021, 1:20 AM

I wonder if there should be a cmake option to switch between the two modes.
Using __func__ has the benefit that it doesn't leak information about the file system where blender is build into the executable. That might also be necessary for reproducable builds. No idea how close we are to that.
The file+line combination can be more useful for debugging though.

intern/guardedalloc/MEM_guardedalloc.h
222	`undef` the utility macros if possible.
224	`LOCATION -> MEM_AT`?

Hans Goudey (HooglyBoogly) added a subscriber: Hans Goudey (HooglyBoogly).Dec 27 2021, 2:56 PM

Hans Goudey (HooglyBoogly) added inline comments.

intern/guardedalloc/MEM_guardedalloc.h
220	`deside` -> `decide`

Fix review issues
Make func the default method

Harbormaster completed remote builds in B19653: Diff 46425.Dec 27 2021, 8:56 PM

I wonder if there should be a cmake option to switch between the two modes.

Not sure if this is possible, I don't think the CMake option will propagate, similar issue with the internationalization macros.

Using __func__ has the benefit that it doesn't leak information about the file system where blender is built into the executable.

Good point, I suppose the default should be __func__ unless we can easily use relative file paths but looking into it, I don't think that's so simple

intern/guardedalloc/MEM_guardedalloc.h
222	Don't think that's possible: https://onlinegdb.com/uao7kdOXs

In D13669#361877, @Jacques Lucke (JacquesLucke) wrote:

Using __func__ has the benefit that it doesn't leak information about the file system where blender is build into the executable. That might also be necessary for reproducable builds. No idea how close we are to that.

WITH_COMPILER_SHORT_FILE_MACRO should resolve that on unix (EDIT linux and mac).

In D13669#362015, @Aaron Carlisle (Blendify) wrote:

I wonder if there should be a cmake option to switch between the two modes.

Not sure if this is possible, I don't think the CMake option will propagate, similar issue with the internationalization macros.

He probably meant a cmake option that conditionally calls a add_definitions to add a define that you can check if it's defined in c code. For example add_definitions(-DWITH_GHOST_DEBUG) and later #ifdef WITH_GHOST_DEBUG . I'm not a fan of these since they make ccache useless. Just a cons to consider.

Pratik Borhade (PratikPB2123) added a subscriber: Pratik Borhade (PratikPB2123).Dec 28 2021, 1:48 PM

It would be nice to have a function where you can leave out the function argument for the allocation name, rather than having to use MEM_AT. Not sure if there is a nice way to do that without std::source_location in C++20 though, would have to change MEM_cnew to be a variadic macro and not use <>.

Rather than MEM_S1/2, you could use STRINGIFY from BLI_utildefines.h.

In D13669#363742, @Brecht Van Lommel (brecht) wrote:

Rather than MEM_S1/2, you could use STRINGIFY from BLI_utildefines.h.

I would prefer not to increase the dependency of BLI on internal modules, I think for the most part they should be self-supporting.

I would prefer not to increase the dependency of BLI on internal modules, I think for the most part they should be self-supporting.

I don't really care much either way, but if not using BLI I suggest to rename to something like MEM_STRINGIFY_ARG and MEM_STRINGIFY for clarity.