[DebugInfo] Fix convert-loclist.ll

[luismarques]

Summary

This test is X86-specific, but it was being run with %llc_dwarf with no target triple. Due to the lack of the target triple, that RUN line would fail on non-x86 platforms (in my case, RISC-V). My understanding is that it would be incorrect to pass a target triple to %llc_dwarf, so this patch fixes this issue by using the plain llc instead, with an x86 target triple.

[dblaikie]

I'm a bit confused - looks like there's a bunch of other uses of %llc_dwarf in this test directory - any idea why those ones don't fail in the same way/for you?

[luismarques]

In D91043#2389617, @dblaikie wrote:

I'm a bit confused - looks like there's a bunch of other uses of %llc_dwarf in this test directory - any idea why those ones don't fail in the same way/for you?

The problem isn't the %llc_dwarf, it's the use of an x86-specific test without an x86 target triple.

The only reason I changed the %llc_dwarf to llc is because I thought that %llc_dwarf shouldn't be used in combination with an explicit target triple command line option. I thought that was the case due to seeing, for instance, commits like [1]. That made some sense to me: the %llc_dwarf would allow you to tweak the triple to ensure proper ELF/DWARF support on certain platforms, so if you explicitly specified the triple then they could clash. Yet, I see one test in that directory that specifies a triple in the RUN line (dwarf-callsite-related-attrs.ll, but maybe that one just needs to be fixed). Most of the tests also specify a triple in the IR, so I guess at least that way it's OK.

So I'm not sure what's the best way to fix this, but the important point is that the backend needs to be told to generate x86 code, otherwise it will trip on the x86-specific attributes and so on.

[1] http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160815/382269.html

[rnk]

This test seems fairly specific, and a bit fragile. I don't think we will get much benefit from trying to generalize it to non-x86 targets. If a triple must be passed, there's no reason to use %llc_dwarf, llc is fine. All %llc_dwarf really does is to set an explicit triple in MSVC environments.

[dblaikie]

In D91043#2390184, @rnk wrote:

This test seems fairly specific, and a bit fragile. I don't think we will get much benefit from trying to generalize it to non-x86 targets.

Sure enough - didn't mean to suggest that we should generalize it.

If a triple must be passed, there's no reason to use %llc_dwarf, llc is fine.

My understanding is that more than that, as @luismarques said, a specific triple + llc_dwarf shouldn't be used together, if I recall correctly. They fail when used together, because llc doesn't accept two triples (doesn't have any support for "pick the last", for instance), maybe/I think/going on past experience/vague recollection?

The fix seems fine to me - but I think there's something that at least I'm not understanding and would like to understand, since it's related to understanding the correctness of this fix: Why are there a bunch of other uses of %llc_dwarf in the DebugInfo/X86 tests - why don't they have the same problem as this test?

[probinson]

In D91043#2390223, @dblaikie wrote:

The fix seems fine to me - but I think there's something that at least I'm not understanding and would like to understand, since it's related to understanding the correctness of this fix: Why are there a bunch of other uses of %llc_dwarf in the DebugInfo/X86 tests - why don't they have the same problem as this test?

I think the first question is rhetorical, but it ensures a DWARF target. The second question is the key: What makes this test special, for @luismarques environment? If you build with just RISC-V as the target, this entire directory should become unsupported. If you have both X86 and RISC-V, then these tests do run, and apparently (mostly) run fine. Why didn't this one?

[luismarques]

In D91043#2406312, @probinson wrote:

I think the first question is rhetorical, but it ensures a DWARF target. The second question is the key: What makes this test special, for @luismarques environment? If you build with just RISC-V as the target, this entire directory should become unsupported. If you have both X86 and RISC-V, then these tests do run, and apparently (mostly) run fine. Why didn't this one?

I think the answer is perhaps that you need *all* of:

1. a non-x86 machine;
2. a build that includes the (non-native) x86 target;
3. a test that is x86-specific, but doesn't specify the x86 target triple nor REQUIRES x86.

Perhaps this test and my build setup was a rare occurrence that matched all 3 requirements?

[dblaikie]

In D91043#2414969, @luismarques wrote:

In D91043#2406312, @probinson wrote:

I think the first question is rhetorical, but it ensures a DWARF target. The second question is the key: What makes this test special, for @luismarques environment? If you build with just RISC-V as the target, this entire directory should become unsupported. If you have both X86 and RISC-V, then these tests do run, and apparently (mostly) run fine. Why didn't this one?

I think the answer is perhaps that you need *all* of:

1. a non-x86 machine;
2. a build that includes the (non-native) x86 target;
3. a test that is x86-specific, but doesn't specify the x86 target triple nor REQUIRES x86.

Perhaps this test and my build setup was a rare occurrence that matched all 3 requirements?

(3) is part of it, but not quite. All these test "REQUIRES" x86 (the DebugInfo/X86 directory has this as a general requirement) - but I did figure out how some tests don't use an -mtriple (so they don't conflict with the llc_dwarf usage): They use a target triple in the IR itself (which I guess doesn't produce an error - not sure if the IR's triple is ignored, or the command line - but one of them seems to be).

So that explains some benign llc_dwarf usage. Though not all of it - I found 7 tests without -mtriple or a target triple in the IR:

llvm/test/DebugInfo/X86/convert-debugloc.ll                                                                                                                                                                                                                                                                                                                             
llvm/test/DebugInfo/X86/convert-inlined.ll                                                                                                                                                                                                                                                                                                                              
llvm/test/DebugInfo/X86/convert-linked.ll                                                                                                                                                                                                                                                                                                                               
llvm/test/DebugInfo/X86/convert-loclist.ll                                                                                                                                                                                                                                                                                                                              
llvm/test/DebugInfo/X86/dbg-byval-parameter.ll            
llvm/test/DebugInfo/X86/debug-macro.ll
llvm/test/DebugInfo/X86/dwarf-callsite-related-attrs.ll

Perhaps the other 6 here are actually not X86 specific & are portable/should be moved to test/DebugInfo (non-X86)... hmm. Seems some are probably portable, others maybe one RUN line in the file is portable. So I've just wholesale switched them all to explicit triples to avoid excess churn - though maybe some of them could've been moved to the generic test bucket in test/DebugInfo directly.

Thanks for your patience, sorry I drew this out a bit further - but glad to better understand what was going on. (mostly it was the IR triples I think that surprised me)

I've committed the cleanup of these 6-7 tests using llc_dwarf without a triple in the metadata (that should close out this review). I'll commit a follow-up commit removing the llc_dwarf from the remaining tests here that rely on the IR target triple, since they don't need the llc_dwarf, I don't think.

[luismarques]

In D91043#2415178, @dblaikie wrote:

(3) is part of it, but not quite. All these test "REQUIRES" x86 (the DebugInfo/X86 directory has this as a general requirement)

I'm not disagreeing with your overall point, but the directory has the check if not 'X86' in config.root.targets, so that checks for target support. When I mentioned the REQUIRES I mean checking for an actual x86 host, not REQUIRES: x86-registered-target.

So that explains some benign llc_dwarf usage. Though not all of it - I found 7 tests without -mtriple or a target triple in the IR:

llvm/test/DebugInfo/X86/convert-debugloc.ll                                                                                                                                                                                                                                                                                                                             
llvm/test/DebugInfo/X86/convert-inlined.ll                                                                                                                                                                                                                                                                                                                              
llvm/test/DebugInfo/X86/convert-linked.ll                                                                                                                                                                                                                                                                                                                               
llvm/test/DebugInfo/X86/convert-loclist.ll                                                                                                                                                                                                                                                                                                                              
llvm/test/DebugInfo/X86/dbg-byval-parameter.ll            
llvm/test/DebugInfo/X86/debug-macro.ll
llvm/test/DebugInfo/X86/dwarf-callsite-related-attrs.ll

Thanks! I noticed that abstract_origin.ll, updated in your commit, does have an IR target triple, and your command-line override only matched it loosely. Perhaps that test wasn't updated quite the way it should have been? (e.g. keep the %llc_dwarf, remove the IR triple, etc.).

[dblaikie]

In D91043#2416003, @luismarques wrote:

In D91043#2415178, @dblaikie wrote:

(3) is part of it, but not quite. All these test "REQUIRES" x86 (the DebugInfo/X86 directory has this as a general requirement)

I'm not disagreeing with your overall point, but the directory has the check if not 'X86' in config.root.targets, so that checks for target support. When I mentioned the REQUIRES I mean checking for an actual x86 host, not REQUIRES: x86-registered-target.

Ah, thanks for the clarification.

So that explains some benign llc_dwarf usage. Though not all of it - I found 7 tests without -mtriple or a target triple in the IR:

llvm/test/DebugInfo/X86/convert-debugloc.ll                                                                                                                                                                                                                                                                                                                             
llvm/test/DebugInfo/X86/convert-inlined.ll                                                                                                                                                                                                                                                                                                                              
llvm/test/DebugInfo/X86/convert-linked.ll                                                                                                                                                                                                                                                                                                                               
llvm/test/DebugInfo/X86/convert-loclist.ll                                                                                                                                                                                                                                                                                                                              
llvm/test/DebugInfo/X86/dbg-byval-parameter.ll            
llvm/test/DebugInfo/X86/debug-macro.ll
llvm/test/DebugInfo/X86/dwarf-callsite-related-attrs.ll

Thanks! I noticed that abstract_origin.ll, updated in your commit, does have an IR target triple, and your command-line override only matched it loosely. Perhaps that test wasn't updated quite the way it should have been? (e.g. keep the %llc_dwarf, remove the IR triple, etc.).

Ah, thanks for the catch! Addressed that in 175ebad958a0ebaf6c56c20ab30b9d4347742c29

llvm/test/DebugInfo/X86/convert-loclist.ll

-; RUN: %llc_dwarf -filetype=obj < %s \
+; RUN: llc -mtriple x86_64-pc-linux -filetype=obj < %s \
 ; RUN:   | llvm-dwarfdump -debug-info -debug-loclists - | FileCheck %s
 ; RUN: llc -mtriple x86_64-pc-linux -split-dwarf-file=foo.dwo -filetype=obj -dwarf-op-convert=Enable < %s \
 ; RUN:   | llvm-dwarfdump -debug-info -debug-loclists - | FileCheck --check-prefix=SPLIT --check-prefix=CHECK %s
 ; RUN: llc -mtriple x86_64-pc-linux -split-dwarf-file=foo.dwo -filetype=asm -dwarf-op-convert=Enable < %s \
 ; RUN:   | FileCheck --check-prefix=ASM %s
 
 ; A bit of a brittle test - this is testing the specific DWO_id. The
 ; alternative would be to test two files with different DW_OP_convert values &