jryans wrote: > This patch adds a pair of new functions in `signals.h` that can be used to > collect and symbolize stack traces respectively. This has major > implementation overlap with the existing stack trace collection/symbolizing > methods, but the existing functions are specialized for dumping a stack trace > to stderr when LLVM crashes, while these new functions are meant to be called > repeatedly during the execution of the program, and therefore we need a > separate set of functions.
This is the main point I was questioning last time, and I am still not sure I follow your thinking here. There's quite a lot of duplicated code, and I believe it would be best for future maintenance if we could reduce that duplication as much as possible. It looks like your new function builds up some data in memory, rather than writing it all to output. Perhaps `printSymbolizedStackTrace` could be reimplemented to call your new `symbolizeAddresses`, thus reducing duplication significantly? Perhaps I've missed some crucial reason why that's hard to do here...? I definitely want to see this work move forward in some form. I'm just trying to reduce future maintenance work where possible. > NB: These functions are only present when origin-tracking is enabled in > CMake. Carrying over a conversation from when this was last put up for > review, where @jryans requested that the functions be made available > unconditionally, I personally believe that since the functions are doing > "unusual" things it would be better to avoid compiling them in until we have > a confirmed second use-case; I could easily be swayed from this if there's a > good argument that another category of users exists, or that in principle > there's not a good reason to hide the functions. Making the new function available unconditionally is less important to me than reducing the code duplication discussed above. It still do believe other users will likely emerge (as I can imagine wanting to use this for some of my own projects), but if you'd rather hide it for now, that's okay with me. (A second use case can be the one to remove the compile guards, for example.) https://github.com/llvm/llvm-project/pull/143591 _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
