renamed get{Nested,}ExpressionLocationsEnabled and moved it into CodeGetModule.cpp

Of course, ApplyDebugLocation is also a perfectly legitimate way to add a debug location to nodes that are not nested inside nodes that already have a location. I updated the diff so that we do end up applying the location in such cases.

added examples suggested by @zturner, verified step over and step into specific behavior matches MSVC, and added tests for them

removed compound statement; that was only in there to double check that the debugger stops on the first child statement, but that's true with or without this change

@zturner: I am still thinking about your comment about other cases to test. My concern is that there are very many possible combinations.

Following conversation with @rnk, I managed to whittle this down to a very small change that seems to do what we need. Step into specific works and single stepping through the program behaves similarly whether compiled with clang-cl or cl.

rnk and I talked about a different approach. The idea is to explicitly emit locations in some cases (e.g. inside compound statements, the braces of for loops, ...), and otherwise emit locations only when emitting column info or emitting non-codeview debug info. That may lead to more elegant code. I'll give it a try later.

I limited the change to only calls, returns, and declarations. I also
updated the test case to include a multi-variable declaration, a while
loop, a for loop, and an if statement (after verifying the behavior in
the debugger, compared to MSVC). I discovered that there is a
difference between the generated info for DWARF with or without
-dwarf-column-info, so I included that in the test, too. I also made a
couple of minor changes that were suggested.

removed accidentally left in include and reformatted mangled comment

lgtm modulo inline comment. Thanks!

Can we ship this?

Use None instead of its overly verbose equivalent.

rewritten using Optional instead of Expected

I'm reverting this change and r311957 to stop the test failures.

The tests are flaky. Running ninja check-llvm multiple times will have them sometimes pass, sometimes fail.

According to http://lab.llvm.org:8011/builders/clang-x86_64-linux-selfhost-modules-2/builds/11192, this is causing tests to fail. I'm investigating.

Size numbers with this version:

rebased

changed 1-bit ints to unsigneds, ran clang-format

I made the change a little more conservative. We now only use the single-record representation
when:

Mostly lgtm; please take a look at the couple of inline comments before landing.

changes requested by @rnk

Thanks for the feedback! I think I incorporated all of it. Does this look good now?

• updated COFF/pieces.ll test to check for records we now emit
• refactored to avoid iterating twice

I still need to update/add tests.

removed stray assert

• rewrite non-deref ranges for variables we changed to references
• factored out interpretation of debug info expressions

@aprantl, I would like to do that, but this doesn't really correspond to any source anymore. I suppose you could take the LLVM IR part of it, take the offsets out of the DIExpressions, and then run it through llc -O0 -stop-after=prologepilog. Then you can edit the DIExpressions to be what you need them to be. Would it be useful to include that in a comment?

Thanks!

How is this for the testcase? It checks positive offsets, negative offsets, and deref.

Thanks for taking a look!

arc diff --verbatim

Added support for DW_OP_deref, and a test case. This is now ready for review.

(Still a work in progress.)

@joerg, any comments?

zturner: Do you have some time to look at this (D33961) and D34013?

I see. Yes, it's unfortunate that this essentially repeats the list of projects in multiple places. On the other hand, I like having as much as possible in the part that can be modified without requiring a buildmaster reload, which would speak for leaving the checkout code in the builder. This is also what the sanitizer builders do. They trigger on changes to any of llvm, cfe, compiler-rt, libcxx, libcxxabi, libunwind, and lld, which we could also do here. It's basically triggering on everything, which is safe. Theoretically, it might increase resource usage a bit, but the ThinLTO builder as-is is basically always building, so the number of builds triggered and the amount of work the worker does should be the same. Would you like me to make that change?

What do you mean by "the scheduling logic gets out of sync"?

Implemented @zturner's suggestions

Thanks! Assuming the tests pass, lgtm

Thanks! lgtm

Hmm, good point. I hadn't considered that changing the name in the same change would mix a lot of mechanical changes with actual interesting to review changes. Let's keep them in two separate changes, then. Can you land them close together in time, though?

Thanks for doing this, zturner! I have some more inline comments and questions.

Oops, I submitted that before I was done reviewing. More comments probably coming soon.

I would like to change the changes to these option names to coincide with renaming llvm-pdbdump to llvm-pdbtool (or some such) so that we only break the interface once.

This change is no longer necessary, right?

The vast majority of changes here are updating tests to expect double quotes instead of single quotes. The actual change to the encoder is in llvm/lib/Support/YAMLTraits.cpp.

lgtm. Thanks!

lgtm

Happy you're doing this, but see inline comments for a few requested changes.

Superseded by D33785

lgtm modulo inline comment about messsage in the assert

I guess I'm saying it seems nice to always write aligned records until we come up with some reason to not write aligned records.

Thanks for fixing this, @zturner! I bumped into this a few days ago, but never really came up with a fix I was confident was the right thing.

pass -j argument to lit if jobs parameter is present

This is not ready for review just yet. There are some other problems with the line information (all offsets show up as 0 when read with the DIA library) that I want to look into a bit more before I'm confident that this change is doing the right thing.