In D67885#1678701, @MyDeveloperDay wrote:

Just as a drive by comment, there is code in DebugInfo/Symbolize/Symbolize.cpp that loads dbghelp via #pragma comment(lib... trick

Updated the patch to simply remove the now redundant workaround.

In D67861#1678618, @labath wrote:

Judging by the microsoft docs, vsnprintf is avaliable at least since VS 2017 (the lowest version supported by llvm). Is it possible that we can just delete this #ifdef ?

@hhb - This is the only change left for builds of lldb on mingw to succeed for me (for x86), but I have a whole bunch of more changes lined up that fix warnings and other issues I've found.

D67887 was committed with the same change.

Just FTR, this was identical to D67857 that I posted a few days ago.

Use static_cast.

Added a comment.

In D67864#1677495, @TomTan wrote:

Thanks for catching this. Could the #pragma push and #undef be moved to the line right before CV_REGISTER(ARM64_FPSR, 220).

In D67856#1677356, @davide wrote:

If this doesn't break clang and gcc, fine.

Updated the CUDA test based on the suggestion.

In D67301#1668327, @mstorsjo wrote:

In D67301#1665612, @MaskRay wrote:

In D67301#1665171, @rnk wrote:

In D67301#1664700, @MaskRay wrote:

I hope at least for ELF, you can keep the "_Z" prefix check. It doesn't need the ObjC "___Z" extension.

Are you sure? Surely users could use -fblocks to get those manglings, or use ObjC on Linux.

We haven't even heard people complaining about lack of demangling of __Z, ___Z or ___Z prefixed names. I still lean towards keeping the "_Z" check for ELF to avoid false positive.

@rnk - do you want to follow up on this further, or should we move forward with this in this form (which is pretty much NFC now).

Can the runtimes repo maintainers @mclow.lists @ldionne @EricWF give this an ack aswell?

In D67053#1665676, @mstorsjo wrote:

Ping @ruiu

Adapted based on the feedback so far, suggestions on naming and grouping the warning are welcome.

In D67414#1668579, @rsmith wrote:

OK. If this is causing compatibility pain in practice, following GCC here seems reasonable. We should at least produce a warning on gnu_inline without extern in C++ mode, though.

In D67414#1668443, @rsmith wrote:

Seems very surprising to me that the gnu_inline attribute has different behavior in their C and C++ frontends. That said, it looks like it's a behavior that they document; their documentation says "In C++, this attribute does not depend on extern in any way, but it still requires the inline keyword to enable its special behavior." and that matches their observed behavior. And they behave this way even for functions in extern "C" blocks. The question for us, then, is do we want to be compatible with C source code built as C++ (that is, the status quo) and C++ source code using this attribute and targeting older versions of Clang, or with g++?

In D67301#1665612, @MaskRay wrote:

In D67301#1665171, @rnk wrote:

In D67301#1664700, @MaskRay wrote:

I hope at least for ELF, you can keep the "_Z" prefix check. It doesn't need the ObjC "___Z" extension.

Are you sure? Surely users could use -fblocks to get those manglings, or use ObjC on Linux.

We haven't even heard people complaining about lack of demangling of __Z, ___Z or ___Z prefixed names. I still lean towards keeping the "_Z" check for ELF to avoid false positive.

Tweaked the syntax for parameter literal names in comments, reinstated a check for config->demangle in the wasm backend.

Removed use of llvm::Optional in the demangle library, adapted COFF/Symbols.cpp to do a check for "demangled != demangleInput" instead of checking an Optional return value.

Ping @ruiu

Extended the llvm demangle function to take an optional parameter for requesting it to only demangle symbols with a stricter prefix check (only allowing _Z).

In D67301#1664700, @MaskRay wrote:

This change is not NFC. This will cause every port of LLD to demangle more names than which were allowed before.

I hope at least for ELF, you can keep the "_Z" prefix check. It doesn't need the ObjC "___Z" extension.

Updated to use the already unified llvm demangler frontend function, removing a lot of duplicated code in lld.

It turns out most of this discussion already is moot; the llvm demangle library already has a common unified function, which takes itanium names with 1-4 leading underscores. That makes this patch even simpler, mostly just rip out a lot of duplicated code from lld. This makes things a bit looser (the previous lld frontend to itaniumDemangle required it to start with exactly _Z), but as this is the current behaviour of the common demangle library, that's probably not an issue in practice.

Renamed the existing functions to clarify that they operate on codeview debug info only.

Reduced the test case further, converted it to use spaces instead of tabs. (A few manual edits had given some parts get mixed indentation in the middle of lines before.) Keeping the symbolizer around in the global config object (without std::unique_ptr, as it's allocated by the arena allocator with make<LLVMSymbolizer>()).

In D67301#1662223, @mstorsjo wrote:

But if we make the demangler implicitly handle both cases, it can also accidentally e.g. try to demangle any symbol which just starts with a capital Z, without any real underscore prefix, on i386 in COFF or in MachO. (I realize the current code in LLD I just committed also has this issue though.)

In D67053#1662604, @MaskRay wrote:

In D67053#1662315, @mstorsjo wrote:

Do you have an opinion on whether I should keep the LLVMSymbolizer object around between calls?

This is the error path and we don't expect this code to be called many times so its contribution to the overall time may be negligible. Keep the same LLVMSymbolizer will benefit when you symbolize the same object multiple times. If reusing LLVMSymbolizer does not take too many lines, you can do that.

In D67053#1662274, @MaskRay wrote:

In D67053#1662244, @mstorsjo wrote:

Reduced the size of the test case by manually cutting out unneeded parts, similar to some of the ELF tests.

The updated test looks good now. Have you had success with larger executables now?

Reduced the size of the test case by manually cutting out unneeded parts, similar to some of the ELF tests.

In D67301#1661748, @rnk wrote:

Should the _ prefix (for itanium symbols in i386 mode) be stripped by the COFF wrapper function, or by the common demangle function (matching _Z or __Z)?

Maybe, I forget if the extra _ comes before the __imp_ prefix or after it when dllimported.