@amccarth: Thanks very much for your review.

Address comments from @amccarth:

In D103089#2791794, @amccarth wrote:

I'm fine focusing on this part to get the bug fixed. We can consider looking at accepting upper and lower case letters in the future.

In D103089#2779878, @amccarth wrote:

I'm be happy to always generate uppercase hex digits when formatting a GUID to registry format, but maybe we should also consider accepting upper or lowercase when reading a formatted GUID.

Correct patch to include binary files used for the test.

In D103089#2779878, @amccarth wrote:

This sounds like an appropriate place to apply Postel's Law: Be conservative in what you do, be liberal in what you accept from others. I'm be happy to always generate uppercase hex digits when formatting a GUID to registry format, but maybe we should also consider accepting upper or lowercase when reading a formatted GUID.

Some clarifications:

• GUIDs are numbers not strings. Microsoft claims they are essentially UUIDs (per RFC 4122).

• The cited document is actually specific to the string representation of GUIDs as required by Windows Installer. In fact, the braces indicate that it's the registry format. Windows installer--and other tools that use formatted GUIDs as registry keys--do string comparisons rather than converting the string representation to a numeric GUID that can be compared directly.

• The bit about using uppercase for the hex digits in the string representation does not apply everywhere, not even in the Microsoft world. Microsoft's GUIDGEN tool formats GUIDs in various ways with respect to the case of the hex digits. And in RFC 4122 says string representations of UUIDs should use lowercase.

In D97411#2611142, @probinson wrote:

In D97411#2598625, @akhuang wrote:

I started looking into some diffs of debug info in libc++ tests, but it's pretty hard to tell what's different - as far as I can see, there are just a bunch of __hash_value_types and __value_types.

This is a job for.... llvm-dva! See the preliminary patch at D88661, although it's getting a bit old and might not apply/build cleanly.

(llvm-dva is undergoing an internal review at the moment, we hope to have a proper reviewable patch series up soon-ish.)

Uploaded a patch that builds with the current TOT.

Uploaded a patch that corrects a merge conflict.

@MaskRay: We have added support for Split DWARF. We would appreciate, if you can try again llvm-dva with the binaries you used in your test cases. Thanks.

Following suggestions from @echristo in relation with the tool name, it was decided to renamed it as llvm-dva.

Uploaded an updated patch:

Uploaded a correct patch that includes:

% fllvm-diva --print=scopes /tmp/Debug/bin/clang             

Logical View:
           {File} '/tmp/debug/bin/clang'

             {CompileUnit} 'cc1_main.cpp.dwo'

             {CompileUnit} 'cc1as_main.cpp.dwo'

             {CompileUnit} 'cc1gen_reproducer_main.cpp.dwo'

             {CompileUnit} 'driver.cpp.dwo'

Are 'instructions', 'symbols', and 'types' unavailable?

% fllvm-diva --print=symbols /tmp/Debug/bin/clang

Logical View:
% fllvm-diva --print=types /tmp/Debug/bin/clang                                                                                                               

Logical View:

Some documented options appear to be unavailable.

% fllvm-diva --sort=offset /tmp/Debug/bin/clang       
llvm-diva: Unknown command line argument '--sort=offset'.  Try: '/tmp/RelA/bin/llvm-diva --help'
llvm-diva: Did you mean '--report=offset'?

For CMake changes, it'd be good to test a -DBUILD_SHARED_LIBS=on configuration. Many unspecified dependency can be caught. It also pushes the author to think whether certain dependencies are needed.

FAILED: lib/libLLVMDebugInfoLogicalView.so.12git
...
ld.lld: error: undefined symbol: llvm::raw_ostream::write(char const*, unsigned long)
ld.lld: error: undefined symbol: llvm::Twine::str[abi:cxx11]() const

@MaskRay Thanks very much for your valuable comments.

In D88661#2307121, @MaskRay wrote:

(If you use arc diff to upload a patch (https://llvm.org/docs/GettingStarted.html#sending-patches), reviewers can use arc patch D88661 to apply the patch locally.
This patch does not have a/ b/ prefixes (git format-patch or git diff) and does not apply cleanly)

This patch includes the a/ b/ prefixes.

In D78265#1999445, @vsk wrote:

Sorry about the breakage, I've renamed the variable in 210616bd38d589020b45f8cbbf9f9ef1296f2729.

In D78265#1999223, @chill wrote:

Looks like it's a scope error in gcc <= 5.5.
https://gcc.godbolt.org/z/ECqRJJ

The minimum required version to build LLVM GCC 5.1.
https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library

In D78265#1999052, @LukeGeeson wrote:

I cannot build llvm locally using ninja clean && ninja check-all with this patch due to the following error:

AArch64ConditionOptimizer.cpp: In member function 'llvm::MachineInstr* {anonymous}::AArch64ConditionOptimizer::findSuitableCompare(llvm::MachineBasicBlock*) ... no matching function call to 'prev(llvm::MachineInstr&)'

Using the following cmake command in llvm-project/build

cmake -G Ninja -DLLVM_ENABLE_PROJECTS="clang;lld" -DCMAKE_BUILD_TYPE=Debug -DLLVM_ENABLE_ASSERTIONS=On -DLLVM_TARGETS_TO_BUILD="ARM;X86;AArch64" -DLLVM_ENABLE_SOLVER_Z3=Off -DPARALLEL_LINK_JOBS=2 -DLLVM_USE_LINKER=gold ../llvm

It looks as though harbourmaster is failing as well.
Based on commit hash 3a53806 on master

In D77184#1960473, @thakis wrote:

I pushed Andrew's fix (thanks!) (with minor formatting tweaks) in dbb0d8ecb3a024bd6817ebd8ad8c5c199a51d933 . Let me know if you still see issues.

In D77184#1957632, @thakis wrote:

Thanks!

@aprantl, as per your feedback I have moved the test case to the correct location.

In D57444#1390350, @dstenb wrote:

Sorry for not noticing this before, but we noticed now that the test case is placed under test/CodeGen/X86/, although it is a simplifycfg test. Should it ideally be placed under test/Transforms/SimplifyCFG/?

Thanks @aprantl, @uabelho for your valuable review.

Updated patch to address comments from @aprantl .

Updated patch to remove the negative CHECK. Thanks for your feedback.

Thanks very much for your invaluable feedback and reviews.

Your change seems to cause build failures. The following is the error message:

@vsk: I have updated the patch to remove the test case pr39187-gline-tables-only.ll, as per your suggestion.

Thanks for your comments.

Thanks for your comments. I have updated the patch to address them.

Ping.

@courbet: Your commit seems to cause build issues:

@aprantl, @vsk: Thanks very much for your valuable comments and suggestions.

In D53287#1274290, @vsk wrote:

LGTM, thanks. I'm sorry for the delay in reviewing this.

@vsk: Do you have any additional comments? It is OK for you if I commit the patch?

Address issues raised by @aprantl.

This revision caused builbot failures.

The patch caused some assertions. For more details see PR39243.

@mstorsjo, The change has been reverted. Thanks for creating the bugzilla.

In D52887#1260298, @mstorsjo wrote:

This broke compilation for me, compiling some source files now fail due to triggered assertions, see PR39243 for details. I guess I'll revert this in a while unless there's someone to fix it soon.

Thanks very much to all the reviewers for your valuable comments and suggestions.

Address issues raised by the reviewers.

Address issues raised by the reviewers.

Address issues raised by the reviewers.

Thanks very much to all the reviewers for your valuable comments and suggestions.

Address issues raised by the reviewers.

In D52614#1248058, @dstenb wrote:

It would be good to change the title to reflect what is done in MCP (at least before submitting this).

Thanks very much to all the reviewers for your valuable comments and suggestions.

The general preference seems to land this patch as-is.

@aprantl and @probinson,

Address issues raised by the reviewers.

In D51976#1235508, @aprantl wrote:

Having read the real testcase now, I understand why you are dropping the debug info now. I would like to point out that we could do better. If we extended llvm.dbg.value to take more than one LLVM SSA Value, then we could produce a DIExpression that selects either %add or %sub depending on the value of %cmp. Let me know if you are interested in implementing this, it should be relatively straight forward to do and would come in useful in all sorts of other situations, too.

Address issues raised by the reviewers.

Address issues raised by the reviewers.

In D51976#1232030, @aprantl wrote:

It still isn't clear to me why you need to drop the debug info in the first place (instead of e.g., making it point to the select, or leave it on its operands) but I hope that that will become obvious once I see the actual testcase.