Microsoft Demangler library

[vuzelac-amd]

This series adds optional MSVC (Microsoft Visual C++) mangled name demangling to the GNU toolchain, using an in-tree library (libdemangle-msvc) that wraps LLVM's MicrosoftDemangle.

The library provides msvc_demangle(), which accepts an MSVC-mangled name and returns the demangled string. The function is accessed from within libiberty just like any other demangling function (access based on style flag, the demangler added to the existing list of demanglers etc.). It's registered externally (using the newly introduced cplus_demangle_set_msvc_handler() ) to avoid licensing issues. The libiberty also sets a new demangling style msvc that tools can use - e.g. cxxfilt, readelf, set demangle-style command in GDB etc.

The library does not provide an equivalent of libiberty's cplus_demangle_v3_components, which returns a structured AST from a mangled name. Implementing one would require converting the LLVM demangler's AST into libiberty's demangle_component tree. Instead, implementation is provided for only two functions that operate on libiberty's AST: GDB's cp_class_name_from_physname & method_name_from_physname. Provided are msvc_class_name_from_physname & msvc_method_name_from_physname and are called DIRECTLY from the aforementioned functions (note that `method_name_from_physname actually has no users in GDB).

The BFD library registers the MSVC demangler from within bfd_init() function, if libdemangle-msvc is configured. This means all the binutils that use libbfd must also link libdemangle-msvc, if the library is configured (see below).

cxxfilt now calls bfd_init() in order to register the MSVC demangler.
readelf now registers the demangler explicitly using cplus_demangle_set_msvc_handler().

Configuration:

• A new configure option --with-msvc-demangler controls whether the libdemangle-msvc library is built. The library is skipped when the flag is set to no, otherwise is built.
• BFD/Binutils/GDB/testsuite link and use libdemangle-msvc if at least one Windows target is enabled during the build or --with-msvc-demangler flag is set.

Testsuite:

• cxxfilt.exp: Added coverage with a probe that skips MSVC tests if built w/o MSVC support.
  Exercise -s msvc / --format=msvc style selection.
• readelf.exp: Added coverage.
• gdb.cp/demangle.exp: Added coverage for msvc_demangle().
  * GDB unittests/cp-support-selftests.c: cover cp_class_name_from_physname & method_name_from_physname.

Docs:

• gdb/NEWS — GDB changes and new demangle-style option.
• CHANGELOG_AMD.md — High-level summary of MSVC demangler integration.
• libdemangle-msvc.texi — Library architecture, registration mechanism, and integration details.
• Binutils documentation — Changes to cxxfilt/readelf and registration via libbfd.

Commits:

• Commit 1 adds LLVM files.
• Commit 2 introduces the libdemangle-msvc and enables it's build using the top level configure script.
• Commits 3, 4, 5, 6 add libdemangle-msvc to libiberty, bfd, gdb and binutils respectively.
• Commit 7 adds the AMD docs.

TODO:
GDB currently calls msvc_class_name_from_physname & msvc_method_name_from_physname. directly if the the symbol appears to be MSVC mangled. A better approach would be to introduce a generic GDB API that calls into libiberty: The libiberty should offer "class-name-from-physname" and "method-from-physname" interfaces that different demanglers can implement. If libiberty has no "native" (fast-path) implementation registered for the type of the symbol received, it returns NULL and the GDB continues with the existing AST-walk and extraction. This effectively moves demangling and style selection to libiberty. This would also need another set of registration functions, along with cplus_demangle_set_msvc_handler().

[czidev-amd]

Hi,
I'll place here my general concerns about this pull request before going on each commit. Here they are:

• making a new demangle routine is not ok. The new MSVC demangling routines need to be called within the old bfd_demangle function.
• I am not very sure the community will approve having the new MSVC demangling library always enabled. Thus, this new library needs to be compiled on demand either by passing an option to the configuration script or by OS target selection.
• adding a llvm library to a gnu project is not something new. We do have llvm libraries added to gcc (i.e., libsanitizers). However, when adding them, we need to make sure those libs are meeting some gnu standards. We should address them sooner than later. For this, we may need to contact the binutils general maintainers to see what the guidelines are.
• another issue is your files are mixing the coding style. I'll try to spot them individually, but you should always run the check_gnu_style script from GCC repo (https://github.com/gcc-mirror/gcc/blob/master/contrib/check_GNU_style.py)
• AFAIK GNU project requires cxx17 which was introduced by gcc8 (experimental). GDB requires minimum gcc7+. Thus, if we introduce cxx17 constructs we need to make sure the gcc version is set to gcc9 or later. This check may be done while configuring the gdb.

[czidev-amd]

General comments on commit e597482 Importing LLVM MSVC Demangler

• How do you plan to maintain this library, do you plan to keep mirroring from llvm project, or have a life of itself within gnu project?
• Probably the name may need to be changed from libdemangle-msvc to libdemangle.
• The llvm folder doesn't seems to be needed, please move everything under it one level up.
• Also the llvm/include folder seems to have another llvm/[Demangle,Config] levels, is it really needed?
• I don't think we need the VENDOR_FROM file. You can add more info in the commit message.
• the commit message needs to be more verbose.
• this commit needs to be combined with parts from next commits. You should look at it as a stand alone commit, which includes msvc sources, adaptation to binutils, testsuite, docs etc.
• You need to combine this commit with next commit (e09be82 Demangler for MSVC symbols), but only the part which configure and builds the lib. Most probably these files mods need to be moved in the e597482 Importing LLVM MSVC Demangler files commit:

libdemangle-msvc/COPYING
libdemangle-msvc/DemangleWrapper.cpp
libdemangle-msvc/Makefile.am
libdemangle-msvc/Makefile.in
libdemangle-msvc/README.md
libdemangle-msvc/aclocal.m4
libdemangle-msvc/config.in
libdemangle-msvc/configure
libdemangle-msvc/configure.ac
libdemangle-msvc/demangle-msvc.cpp
libdemangle-msvc/demangle-msvc.h
libdemangle-msvc/testsuite/Makefile.in
libdemangle-msvc/testsuite/msvc-demangle-expected
libdemangle-msvc/testsuite/test-msvc-demangle.cpp

Makefile.def
Makefile.in

• Also, you need to make this library to be compiled optionally (i.e., add configure option --with-libdemangler)
• The README.md should be moved into a doc/ folder, and converted to texi. Also, it should be build and installed when the documentation of the binutils is produced (if the library is enabled).

The purpose here is to get a single commit which adds and integrates the new libdemangler into binutils-gdb source tree.

[czidev-amd]

General comments on commit e09be82 Demangler for MSVC symbols.

As I showed earlier (#92 (comment)), this commit needs to be refactored. Parts of it needs to be merged with the initial pull of the libdemangler-mscv. The rest needs to add support to demangle MSVC symbols in bfd's bfd_demangle function.
To do so, please remove the newly introduced bfd_demangle_new function, and investigate if it is possible to have all the MSVC demangling without changing the prototype of the bfd_demangle function.
Probably, as a result of the investigation the next commmit (84d326b Add LIBICONV to readelf, dllwrap, elfedit, nm, objdump, addr2line, cxxfilt) may be moved to this revamped commit. Possibly the last commit too, if the bfd_demangling mods are touching everywhere.

[vuzelac-amd]

Hi, I'll place here my general concerns about this pull request before going on each commit. Here they are:

• making a new demangle routine is not ok. The new MSVC demangling routines need to be called within the old bfd_demangle function.
• I am not very sure the community will approve having the new MSVC demangling library always enabled. Thus, this new library needs to be compiled on demand either by passing an option to the configuration script or by OS target selection.
• adding a llvm library to a gnu project is not something new. We do have llvm libraries added to gcc (i.e., libsanitizers). However, when adding them, we need to make sure those libs are meeting some gnu standards. We should address them sooner than later. For this, we may need to contact the binutils general maintainers to see what the guidelines are.
• another issue is your files are mixing the coding style. I'll try to spot them individually, but you should always run the check_gnu_style script from GCC repo (https://github.com/gcc-mirror/gcc/blob/master/contrib/check_GNU_style.py)
• AFAIK GNU project requires cxx17 which was introduced by gcc8 (experimental). GDB requires minimum gcc7+. Thus, if we introduce cxx17 constructs we need to make sure the gcc version is set to gcc9 or later. This check may be done while configuring the gdb.

1. bfd_init now registers msvc_demangle into libiberty using a new API - cplus_demangle_set_msvc_handler().
2. libdemangle-msvc configured if a windows target is used or --with-msvc-demangler configure flag is set.
3. Checked with check_GNU_style.py
4. gdb/configure.ac: AX_CXX_COMPILE_STDCXX(17, , mandatory). GDB already requires C++17 as mandatory ?

[vuzelac-amd]

General comments on commit e597482 Importing LLVM MSVC Demangler

• How do you plan to maintain this library, do you plan to keep mirroring from llvm project, or have a life of itself within gnu project?
• Probably the name may need to be changed from libdemangle-msvc to libdemangle.
• The llvm folder doesn't seems to be needed, please move everything under it one level up.
• Also the llvm/include folder seems to have another llvm/[Demangle,Config] levels, is it really needed?
• I don't think we need the VENDOR_FROM file. You can add more info in the commit message.
• the commit message needs to be more verbose.
• this commit needs to be combined with parts from next commits. You should look at it as a stand alone commit, which includes msvc sources, adaptation to binutils, testsuite, docs etc.

Thus,

• You need to combine this commit with next commit (e09be82 Demangler for MSVC symbols), but only the part which configure and builds the lib. Most probably these files mods need to be moved in the e597482 Importing LLVM MSVC Demangler files commit:

libdemangle-msvc/COPYING
libdemangle-msvc/DemangleWrapper.cpp
libdemangle-msvc/Makefile.am
libdemangle-msvc/Makefile.in
libdemangle-msvc/README.md
libdemangle-msvc/aclocal.m4
libdemangle-msvc/config.in
libdemangle-msvc/configure
libdemangle-msvc/configure.ac
libdemangle-msvc/demangle-msvc.cpp
libdemangle-msvc/demangle-msvc.h
libdemangle-msvc/testsuite/Makefile.in
libdemangle-msvc/testsuite/msvc-demangle-expected
libdemangle-msvc/testsuite/test-msvc-demangle.cpp

Makefile.def
Makefile.in

• Also, you need to make this library to be compiled optionally (i.e., add configure option --with-libdemangler)
• The README.md should be moved into a doc/ folder, and converted to texi. Also, it should be build and installed when the documentation of the binutils is produced (if the library is enabled).

The purpose here is to get a single commit which adds and integrates the new libdemangler into binutils-gdb source tree.

1. life of itself within gnu project, that is, manual update...
2. llvm folder - I wanted to separate imported files so it's easier to mantain. Is that ok ?
3. llvm/[Demangle,Config]: Config is removed (no includes from there: llvm/Demangle kept in order not to edit LLVM files.
4. VENDOR_FROM removed
5. Commit messages updated.
6. Combining commits: Everything is reorganized now. Commit 1 adds LLVM files, Commit 2 introduces the libdemangle-msvc and enables it's built using the top level configure script. Commits 3, 4, 5, 6 add libdemangle-msvc to libiberty, bfd, gdb and binutils respectively. Commit 7 updates AMD docs.
7. Compiled optionally.
8. README moved to texi. TODO: Optional compile

[vuzelac-amd]

General comments on commit e09be82 Demangler for MSVC symbols.

As I showed earlier (#92 (comment)), this commit needs to be refactored. Parts of it needs to be merged with the initial pull of the libdemangler-mscv. The rest needs to add support to demangle MSVC symbols in bfd's bfd_demangle function. To do so, please remove the newly introduced bfd_demangle_new function, and investigate if it is possible to have all the MSVC demangling without changing the prototype of the bfd_demangle function. Probably, as a result of the investigation the next commmit (84d326b Add LIBICONV to readelf, dllwrap, elfedit, nm, objdump, addr2line, cxxfilt) may be moved to this revamped commit. Possibly the last commit too, if the bfd_demangling mods are touching everywhere.

LIBCONV fix adds libconv to mingw builds (not added by default). Unrelated to libdemangle-msvc. The commit moved to the begining.

[lancesix]

• AFAIK GNU project requires cxx17 which was introduced by gcc8 (experimental). GDB requires minimum gcc7+. Thus, if we introduce cxx17 constructs we need to make sure the gcc version is set to gcc9 or later. This check may be done while configuring the gdb.

4. gdb/configure.ac: `AX_CXX_COMPILE_STDCXX(17, , mandatory)`. GDB already requires C++17 as mandatory ?

Yes, GDB already requires C++17 (has been the case for 2 years or so).

[palves]

Does the libiconv change really have anything to do with the demangler?

The log of commit:

"Add LIBICONV to readelf, dllwrap, elfedit, nm, objdump, addr2line, cxxfilt."

doesn't say. Why is that patch in this series? Also, config/gettext.m4 should already be handling adding LIBICONV. What's going on?

[vuzelac-amd]

Does the libiconv change really have anything to do with the demangler?

The log of commit:

"Add LIBICONV to readelf, dllwrap, elfedit, nm, objdump, addr2line, cxxfilt."

doesn't say. Why is that patch in this series? Also, config/gettext.m4 should already be handling adding LIBICONV. What's going on?

It was supposed to be a dependency/prerequisite for building binutils thus it was a separate commit.
As for this being a dependency: Duh, of course it's not - mingw surely builds binutils. I don't recall now what issue I had. Commit REMOVED.

[czidev-amd]

We have only the static version build here; don't we need to have a shared library too? If yes, don't forget to add the library version information too.

[vuzelac-amd]

If it gets linked into libbfd then I guess we don't need the static lib.
Otherwise, I am looking into other libs namely libiberty and there also is not shared lib equivalent. I would not consider this library any different.

[palves]

• llvm folder - I wanted to separate imported files so it's easier to mantain. Is that ok ?

Yes, please. That follows the model we use for the gnulib/ directory, where the imported code is in the gnulib/import/ subdirectory. Very easy to tell what is imported and what is maintained downstream that way.

• Combining commits: Everything is reorganized now. Commit 1 introduces the libdemangle-msvc and enables building it; configure.ac in gdb/binutils will parse --with-msvc-demangler while top level Makefile adds the library. Commits 2,3,4 add libdemangle-msvc to BFD, BINUTILS and GDB respectively.

I don't recall how it was before, but one thing that I think is important that I had requested before is missing. I would very much like to keep the import of the llvm files separate from anything else, so that we can see your local changes to the llvm files as a separate commit. It will be easier to review/discuss those, an also, it'll be easier in the future when we next need to reimport import changes from llvm, or even wholesale reimport a new version.

So I'd much prefer a patch/commit that first imports the llvm files into libdemangle-msvc/llvm, and then a second commit that adds the glue plus your local changes, at least.

I don't see how it makes sense to have changes to binutils/'s configury in the "libdemangle-msvc: introduce MSVC symbol demangling library" patch, and then "libdemangle-msvc: use MSVC demangler in binutils" is the patch that actually makes use of libdemangle-msvc in binutils, and, it even changes the --enable-msvc-demangler options added by the first patch to --with-msvc-demangler. That really hints at a bad split. Please move the binutils configure changes from the first patch to the binutils patch. Same for the gdb changes.

You have a lot of spurious space/tab / trailing spaces. Add this to your ~/.gitconfig:

[core]
        whitespace = trailing-space,space-before-tab

and then "git show/diff" will show you the bad space/tabs in red.

Why do all tools that use bfd need to call bfd_set_msvc_demangler, instead of bfd doing it if libdemangle-msvc support is enabled in bfd?

The use of CLANG_FOR_TARGET in the binutils tests is a bit too hacky. We should really switch to configuring the toolchain with --target=x86_64-pc-windows-msvc, and then you'd just use a normal istarget check to skip of not "istarget *-windows-msvc". But, OTOH, why do you need the compiler at all here? libiberty/testsuite/demangle-expected doesn't need one. Can't we do the same?

The msvc demangler should be enabled by default at least when targeting windows.

This:

+if test "x${enable_msvc_demangler}" != "xno"; then
+    AC_DEFINE(HAVE_MSVC_DEMANGLER, 1,
+             [defined if MSVC demangler was requested.])
+    LIBDEMANGLE_MSVC_LIBS='-lstdc++'
+else
+    LIBDEMANGLE_MSVC_LIBS=
+fi
+AC_SUBST(LIBDEMANGLE_MSVC_LIBS)

... is assuming you're using a libstdc++ toolchain (i.e. GCC or Clang + libstdc++). There are other C++ runtime libraries, e.g., libc++ from llvm. The right way I think is to link with the C++ compiler instead of the C compiler if the msvc demangled is enabled.

[vuzelac-amd]

• llvm folder - I wanted to separate imported files so it's easier to mantain. Is that ok ?

Yes, please. That follows the model we use for the gnulib/ directory, where the imported code is in the gnulib/import/ subdirectory. Very easy to tell what is imported and what is maintained downstream that way.

Done

• Combining commits: Everything is reorganized now. Commit 1 introduces the libdemangle-msvc and enables building it; configure.ac in gdb/binutils will parse --with-msvc-demangler while top level Makefile adds the library. Commits 2,3,4 add libdemangle-msvc to BFD, BINUTILS and GDB respectively.

I don't recall how it was before, but one thing that I think is important that I had requested before is missing. I would very much like to keep the import of the llvm files separate from anything else, so that we can see your local changes to the llvm files as a separate commit. It will be easier to review/discuss those, an also, it'll be easier in the future when we next need to reimport import changes from llvm, or even wholesale reimport a new version.

Please see the PR updated description and the

So I'd much prefer a patch/commit that first imports the llvm files into libdemangle-msvc/llvm, and then a second commit that adds the glue plus your local changes, at least.

I don't see how it makes sense to have changes to binutils/'s configury in the "libdemangle-msvc: introduce MSVC symbol demangling library" patch, and then "libdemangle-msvc: use MSVC demangler in binutils" is the patch that actually makes use of libdemangle-msvc in binutils, and, it even changes the --enable-msvc-demangler options added by the first patch to --with-msvc-demangler. That really hints at a bad split. Please move the binutils configure changes from the first patch to the binutils patch. Same for the gdb changes.

This mess is cleaned up now.

You have a lot of spurious space/tab / trailing spaces. Add this to your ~/.gitconfig:

[core]
        whitespace = trailing-space,space-before-tab

Done

and then "git show/diff" will show you the bad space/tabs in red.

Why do all tools that use bfd need to call bfd_set_msvc_demangler, instead of bfd doing it if libdemangle-msvc support is enabled in bfd?

Demangler back into libiberty, registered externally by bfd_init(). Still, cxxfilt needs to add the call to bfd_init() and readelf needs to register the library with libiberty, as it doesn't use libbfd (an option is to link readelf w/ libiberty if libdemangle-msvc is configured)>

The use of CLANG_FOR_TARGET in the binutils tests is a bit too hacky. We should really switch to configuring the toolchain with --target=x86_64-pc-windows-msvc, and then you'd just use a normal istarget check to skip of not "istarget *-windows-msvc". But, OTOH, why do you need the compiler at all here? libiberty/testsuite/demangle-expected doesn't need one. Can't we do the same?

Since the msvc demangler is now back in libiberty and it's registered externally by BFD, the tests are now removed except for cxxfilt testing which need no compilation, and readelf testing that doesn't need any new target/compiler.

The msvc demangler should be enabled by default at least when targeting windows.

A new configure option --with-msvc-demangler controls whether the MSVC demangling library is built. The library is not build if this option explicitly disables the library. gdb/binutils/BFD link and use the library if a Windows target is selected or --with-msvc-demangler flag is set. @czidev-amd

This:

+if test "x${enable_msvc_demangler}" != "xno"; then
+    AC_DEFINE(HAVE_MSVC_DEMANGLER, 1,
+             [defined if MSVC demangler was requested.])
+    LIBDEMANGLE_MSVC_LIBS='-lstdc++'
+else
+    LIBDEMANGLE_MSVC_LIBS=
+fi
+AC_SUBST(LIBDEMANGLE_MSVC_LIBS)

... is assuming you're using a libstdc++ toolchain (i.e. GCC or Clang + libstdc++). There are other C++ runtime libraries, e.g., libc++ from llvm. The right way I think is to link with the C++ compiler instead of the C compiler if the msvc demangled is enabled.

Introducing CXXLINK command that follows LINK built by automake

if ENABLE_MSVC_DEMANGLER
BINUTILS_LINK = $(LIBTOOL) $(AM_V_lt) --tag=CXX $(AM_LIBTOOLFLAGS) \
	$(LIBTOOLFLAGS) --mode=link $(CXX) $(AM_CXXFLAGS) $(CXXFLAGS) \
	$(AM_LDFLAGS) $(LDFLAGS) -o $@
else
BINUTILS_LINK = $(LINK)
endif

[vuzelac-amd]

@czidev-amd @palves All comments should be addressed.

[czidev-amd]

Looks better, You still need to fix CXX issue for binutils.

[czidev-amd]

This requires every binutils build a C++ compiler. this can be very problematic request as it may change build systems across different distro providers. I would at least add a guarding if ENABLE_MSVC_DEMANGLER statement.

You need to check if everything works like before without CXX compiler: (CXX=false ./configure ...) at least for binutils.

[vuzelac-amd]

Actually the way automake works I can't make inclusion of cxx files conditional. Switched to the new link command for binutils:

if ENABLE_MSVC_DEMANGLER
BINUTILS_LINK = $(LIBTOOL) $(AM_V_lt) --tag=CXX $(AM_LIBTOOLFLAGS) \
	$(LIBTOOLFLAGS) --mode=link $(CXX) $(AM_CXXFLAGS) $(CXXFLAGS) \
	$(AM_LDFLAGS) $(LDFLAGS) -o $@
else
BINUTILS_LINK = $(LINK)
endif

objdump_LINK = $(BINUTILS_LINK)
nm_new_LINK = $(BINUTILS_LINK)
...

[czidev-amd]

Typo, bracket opening ] instead of [