In D59620#1437718, @aganea wrote:

@rnk At a later stage, would it be possible to add an option to only print the referenced/unreferenced types? ie. llvm-pdbutil dump -types=[all(default),ref,unref]

lgtm

In D59620#1438238, @zturner wrote:

The reason I put the variable with the decltype is because it's the easiest way to force the debugger to deal with the cycle. Without that, sure you can generate a cycle in the debug info, but it's not clear under what conditions the debugger would actually require that record.

This way, it's easy to add a watch in the debugger for foo.node and the debugger is forced to resolve the cycle.

In D59620#1438190, @zturner wrote:

I don't think you can have a cycle to an unnamed type though because it's impossible to reference it.

In D59620#1438062, @zturner wrote:

In D59620#1438019, @aganea wrote:

Another question - do you think we could replace forward references by the the concrete ones?

I don't think we could replace forward references with the full declarations. Forward references exist to deal with cycles, as well as to make sure the entire type stream can be topologically sorted.

lgtm

I see! I only took a quick look, but this looks exactly like what @rsmith has been asking for in discussions for a long time now: a more explicit AST representation of uuid of uuidof in template arguments. I'll make an effort to get his attention and see if this addresses his concerns.

The skeleton of the dumper seems fine.

Aside from the buglet, I'm happy with this, but I wanted to wait until @george.burgess.iv takes a look.

I think this is worth the complexity of the repeated clone methods. lgtm

lgtm

Apparently I wrote this comment long ago and never hit send.

Looks good to me. Let's wait for @jyknight though.

I think this is a good change. The map on the side is less string-ly typed now. We should wait for @ruiu since it was his review feedback.

lgtm

Thanks! I haven't read the code in detail, but this all sounds good to me. (/me goes back to cave).

I agree with your analysis, thanks!

@aprantl, I think you are correct for all the code that I can find today. The only thing that matters is the inlinedAt part of the location. But I did see this code that handles dbg.declare, which uses the scope from the location:
https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1042

Very cool! I'll take a look, I wasn't aware this had been rebased and uploaded, I was thinking about doing it myself yesterday as a side project.

This makes me nervous. The pair of the DIVariable and DILocation is used to uniquely identify the variable that a dbg.value applies to. In practice, inlining is usually what creates multiple instances of the same variable that get updated by dbg.values. You can see how the location is used in DwarfDebug::collectVariableInfoFromMFTable.

I don't think this change is correct, the test is creating a global variable, run_on_startup, and allocating it into .CRT$XIB. There are no initializers involved, which is what #pragma init_seg controls. We'd have to restructure the test like so for that to make sense:

#pragma init_seg(".crt$XIB")
struct AutoIniti { AutoInit() { call_me_maybe(); } } run_on_startup;

This test is essentially rolling its own initializer, so data_seg is the right pragma. Another way to do this would be to use #pragma section / __declspec(allocate).

lgtm

Looks good, I see why this is needed now.

@gbiv already got all my shell quoting comments.

Yep, as a direction, this makes sense to me.

Lgtm!

lgtm, thanks! Would you like someone to land this for you?

lgtm

lgtm

• rebase

lgtm

lgtm

Should this be factored into some general define in some header (which one?) like SANITIZER_WIN_TRACE or so, to avoid duplicating the condition all over the place?

lgtm

In D58651#1413884, @yln wrote:

Is there a way to emulate weak linkage on windows?
Can you offer guidance on how to resolve this?

lgtm

lgtm

I'd prefer to keep the same /Zi vs. /Z7 setting in the standalone build. Does the standalone build not have access to the installed LLVM cmake modules? If so, I would think we could include them. Or, we could put in a basic regex here like we used to have for -DNDEBUG.

I experimented with this program to confirm the behavior matches MSVCRT:

#include <stdio.h>
extern "C" const char *GetCommandLineA(void);
int main(int argc, char **argv) {
  puts(GetCommandLineA());
  for (int i = 0; i < argc; ++i) {
    puts(argv[i]);
  }
}

In D58737#1412847, @xur wrote:

lgtm.

BTW, I'm in the process of committing D54175. After that patch, PGO instrumentation can be called after the main inlining. I don't think it will conflict anything in this patch

Oops, forgot to respond to this...

• share code

In D58737#1412734, @vsk wrote:

I'm confused by this wording re: comdats in the LangRef: "All global objects that specify this key will only end up in the final object file if the linker chooses that key over some other key". Why can multiple global objects with the same comdat key end up in the final object file?

Also, why is the selection kind here 'any' instead of 'exactmatch'? If the counter array sizes legitimately can vary (why?), then wouldn't we want 'largestsize'?

In D57982#1412610, @kcc wrote:

So, apparently, checking for isa<UnreachableInst>(DestBB->getTerminator()) is not the right way to check if the block entry is unreachable.

Thoughts?

In D58599#1410086, @aganea wrote:> The following does not produce the expected result - instead the text is half-printed on the console, and the dump file only contains partial information (Discarded symbols and the cmd-line):

lld-link.exe ... /verbose >dump.txt

lgtm

In D57982#1412373, @vitalybuka wrote:

In D57982#1412322, @morehouse wrote:

Sure, but that block is generally still not useful to instrument (for fuzzing). When fuzzing, we save all inputs that either (1) increase coverage as measured by SanitizerCoverage, or (2) crash. So if case 2 happens every time we touch a block that ends in unreachable, there's no point in instrumenting it so that case 1 happens too.

This explanation looks reasonable to me.

In D58694#1411211, @anhtuyen wrote:

Removing an attribute explicitly added by the user (writeonly) and replacing it with the attribute we deduced (readnone) ? That might not be the improvement we want.
A side note, but the snippet was reduced from an actual code, where the writeonly is required.

@vitalybuka @morehouse, any thoughts on this comment:
https://github.com/llvm/llvm-project/blob/29ac3a5b822ba8c097a3ae78d983cdb94da43dd4/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp#L457

This seems like the wrong fix, I would expect functionattrs to improve the deduction by removing the writeonly attribute.