Re: CVS commit: src/sys/lib/libunwind
Joerg, I still think this import was a bad engineering decision and should be backed out, followed by a proposal, discussion and then re-import in modified form. First: it is all fine for /usr/src/lib/*, if you need it in userland for clang, it could live there (unmodified) for now. But for kernel, I think a better design is needed. Here is a quick sketch of how I would start looking at a design: Goal: use libunwind mostly as-is to provide the C++ ABI for exception handling and unwind operations (joerg's pointer to the Itanium documents). Share parts of this with the kernel, so the unwind info can be used e.g. inside ddb. The shared-with-kernel part should be written in plain C, and use abstractions already present in our code - see arch/mcontext.h, use __gregset_t, _NGREG32, _REG_PC and friends. If this part needs MD code, the already present kernel functions should be used so for the kernel part no additional MD work is needed. This means re-implementing (or porting) the DWARF parser in C, plus a bit of glue. The userland C++ code can reuse this, with maybe a few helper functions needed. The interface of the share part should be separately documented (as there is no use for the C++ unwind/exception ABI inside the kernel). Martin
Re: CVS commit: src/sys/lib/libunwind
On Thu, Oct 17, 2013 at 09:17:38AM +0200, Martin Husemann wrote: But for kernel, I think a better design is needed. I completely disagree that any of this provides an actual improvement. Joerg
Re: CVS commit: src/sys/lib/libunwind
joerg@ wrote: On Wed, Oct 16, 2013 at 11:36:18AM +, Martin Husemann wrote: On Mon, Oct 14, 2013 at 01:14:58AM +, Joerg Sonnenberger wrote: Module Name: src Committed By: joerg Date: Mon Oct 14 01:14:58 UTC 2013 Added Files: src/sys/lib/libunwind: AddressSpace.hpp CREDITS.TXT DwarfInstructions.hpp DwarfParser.hpp LICENSE.TXT Makefile.inc Registers.hpp UnwindCursor.hpp dwarf2.h libunwind.cxx unwind.h unwind_registers.S Log Message: Add a heavily modified version of Apple's libunwind as released under MIT license in libc++abi. At the moment, only x86 support is tested. A stealth import is no use in bypassing the standard questions: - where has this import been publically discussed? The need was mentioned a number of times in various threads about libc++ and clang. Why do you always hide actual pointers (mailing list archive URLs etc.) to the prior discussions you claim? That's the reason why people think and complain your commit is a stealth import. Anyway, our commit guidelines explicitly require Core's approval before adding a new package into base. You violate the rule. That's the enough reason to revert your commit without discussion. -- Izumi Tsutsui
Re: CVS commit: src/sys/lib/libunwind
On Thu, Oct 17, 2013 at 08:57:23PM +0900, Izumi Tsutsui wrote: Anyway, our commit guidelines explicitly require Core's approval before adding a new package into base. You violate the rule. I didn't. Joerg
Re: CVS commit: src/sys/lib/libunwind
On Oct 17, 2013, at 4:57 AM, Izumi Tsutsui tsut...@ceres.dti.ne.jp wrote: Anyway, our commit guidelines explicitly require Core's approval before adding a new package into base. You violate the rule. That's the enough reason to revert your commit without discussion. He did get core's approval and core did require some changes to the import. We did ask why not sys/external and other things. Given the reasons given, we thought sys/lib/libunwind was the best place so it can be used both in the kernel and userland. If you've ever looked at the stacktrace code for ARM or MIPS, any alternative would be better than the existing code. Being able to -fomit-stack-pointer for performance and still get a ddb stacktrace is also a win.
Re: CVS commit: src/sys/lib/libunwind
On Oct 17, 2013, at 4:57 AM, Izumi Tsutsui tsut...@ceres.dti.ne.jp wrote: Anyway, our commit guidelines explicitly require Core's approval before adding a new package into base. You violate the rule. That's the enough reason to revert your commit without discussion. He did get core's approval and core did require some changes to the import. We did ask why not sys/external and other things. Given the reasons given, we thought sys/lib/libunwind was the best place so it can be used both in the kernel and userland. Then approval without public discussion? It looks most core members always consider only technical points and often ignore all other things, like communication, procedures, and marketing issue etc. It really disappoints other developers and many users... --- Izumi Tsutsui
Re: CVS commit: src/sys/lib/libunwind
On Mon, Oct 14, 2013 at 01:14:58AM +, Joerg Sonnenberger wrote: Module Name: src Committed By: joerg Date: Mon Oct 14 01:14:58 UTC 2013 Added Files: src/sys/lib/libunwind: AddressSpace.hpp CREDITS.TXT DwarfInstructions.hpp DwarfParser.hpp LICENSE.TXT Makefile.inc Registers.hpp UnwindCursor.hpp dwarf2.h libunwind.cxx unwind.h unwind_registers.S Log Message: Add a heavily modified version of Apple's libunwind as released under MIT license in libc++abi. At the moment, only x86 support is tested. A stealth import is no use in bypassing the standard questions: - where has this import been publically discussed? - why is it placed in src/sys/lib instead of src/sys/external/...? - there is no user in tree yet, why the import upfront? - what are the alternatives to this very ugly, undocumented, templated C++ code? Martin
Re: CVS commit: src/sys/lib/libunwind
On Wed, Oct 16, 2013 at 11:36:18AM +, Martin Husemann wrote: On Mon, Oct 14, 2013 at 01:14:58AM +, Joerg Sonnenberger wrote: Module Name:src Committed By: joerg Date: Mon Oct 14 01:14:58 UTC 2013 Added Files: src/sys/lib/libunwind: AddressSpace.hpp CREDITS.TXT DwarfInstructions.hpp DwarfParser.hpp LICENSE.TXT Makefile.inc Registers.hpp UnwindCursor.hpp dwarf2.h libunwind.cxx unwind.h unwind_registers.S Log Message: Add a heavily modified version of Apple's libunwind as released under MIT license in libc++abi. At the moment, only x86 support is tested. A stealth import is no use in bypassing the standard questions: - where has this import been publically discussed? The need was mentioned a number of times in various threads about libc++ and clang. - why is it placed in src/sys/lib instead of src/sys/external/...? Because it is not handled as 3rd party source. - there is no user in tree yet, why the import upfront? This is not true. - what are the alternatives to this very ugly, undocumented, templated C++ code? I won't ever both with this such an opinionated question... Joerg
Re: CVS commit: src/sys/lib/libunwind
On Wed, Oct 16, 2013 at 05:09:25PM +0200, Joerg Sonnenberger wrote: The need was mentioned a number of times in various threads about libc++ and clang. But for the kernel? And the details? Like where to place it and what to use it for? - why is it placed in src/sys/lib instead of src/sys/external/...? Because it is not handled as 3rd party source. Why not? And what does heavily modified (from the import log) mean? - there is no user in tree yet, why the import upfront? This is not true. Give me hint, please. - what are the alternatives to this very ugly, undocumented, templated C++ code? I won't ever both with this such an opinionated question... Very true - but the point is, that it would be realy helpfull if you could not drop such easter eggs somewhere hidden in the tree, but explain reasoning and plans upfront. I am not a C++ hater, as you probably know, but C++, with templates, and not even a C API, apparently prepared to be used in the kernel -- IMHO deserves an introductionary mail, at the very least. Martin
Re: CVS commit: src/sys/lib/libunwind
On Wed, Oct 16, 2013 at 05:22:15PM +0200, Martin Husemann wrote: On Wed, Oct 16, 2013 at 05:09:25PM +0200, Joerg Sonnenberger wrote: The need was mentioned a number of times in various threads about libc++ and clang. But for the kernel? And the details? Like where to place it and what to use it for? It will be used (optionally) by DDB to allow backtraces even in the presence of -fomit-frame-pointer. - why is it placed in src/sys/lib instead of src/sys/external/...? Because it is not handled as 3rd party source. Why not? And what does heavily modified (from the import log) mean? Unnecessary parts like the support for Apple's Compact Unwind format are ripped out, HP's libunwind interface is not present etc. - there is no user in tree yet, why the import upfront? This is not true. Give me hint, please. Look at lib/libc/Makefile? - what are the alternatives to this very ugly, undocumented, templated C++ code? I won't ever both with this such an opinionated question... Very true - but the point is, that it would be realy helpfull if you could not drop such easter eggs somewhere hidden in the tree, but explain reasoning and plans upfront. I am not a C++ hater, as you probably know, but C++, with templates, and not even a C API, apparently prepared to be used in the kernel -- IMHO deserves an introductionary mail, at the very least. The user interface is plain C, the rest is just an implementation detail. The C interface creates instances of the DWARF logic, the address space abstraction and the register set abstraction. The address space is a parameter to allow using it in contexts like core dumps or separate processes. The register set is a parameter for dealing with 32bit vs 64bit on a platform for example. The template use allows all this without additional overhead at run time, which would not be possible without a lot of mess otherwise. HP's libunwind is a nice case study for that... Joerg
Re: CVS commit: src/sys/lib/libunwind
On Wed, Oct 16, 2013 at 05:35:54PM +0200, Joerg Sonnenberger wrote: Look at lib/libc/Makefile? Ok, I see it is build into libc on some archs with clang, but where is it called? The user interface is plain C, the rest is just an implementation detail. The C interface creates instances of the DWARF logic, the address space abstraction and the register set abstraction. I realy tried to find this user interface, but I failed. There are like 20 functions declared in unwind.h, with speaking names like _Unwind_GetGR and no further documentation. And then all MD parts are mangled into a single header plus a single asm file with spagethi ifdef cascades. Apparently for extending to other archs you are expected to create some asm functions which do not even have their C++ signature documented and where you have to start looking for the coresponding line in some header file by demengling the decorated C++ name. I realy think this should be heavily rearranged, and the C++ stuff evicted from kernel land completely - even if that means we have to copy some things or use evil macros instead of evil templates. Martin
Re: CVS commit: src/sys/lib/libunwind
On Wed, Oct 16, 2013 at 06:00:56PM +0200, Martin Husemann wrote: On Wed, Oct 16, 2013 at 05:35:54PM +0200, Joerg Sonnenberger wrote: Look at lib/libc/Makefile? Ok, I see it is build into libc on some archs with clang, but where is it called? libexecinfo, libc++, otherwise libraries that do stack unwinding for exception handling like libobjc. The user interface is plain C, the rest is just an implementation detail. The C interface creates instances of the DWARF logic, the address space abstraction and the register set abstraction. I realy tried to find this user interface, but I failed. There are like 20 functions declared in unwind.h, with speaking names like _Unwind_GetGR and no furtherdocumentation. See link at the beginning of libunwind.cxx. Joerg
Re: CVS commit: src/sys/lib/libunwind
On Wed, Oct 16, 2013 at 06:03:57PM +0200, Joerg Sonnenberger wrote: See link at the beginning of libunwind.cxx. The documentation should in the source tree and should be manual pages. Thor
