[PATCH] D61996: [llvm-objdump]Improve testing of some switches #2

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objdump/X86/Inputs/source-interleave.ll:2
+; ModuleID = 'source-interleave-x86_64.bc'
+source_filename = "source-interleave-x86_64.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
----------------
MaskRay wrote:
> jhenderson wrote:
> > MaskRay wrote:
> > > Rename..
> > Rename which bit? I can't rename the file to two different files (as things stand I've renamed X86/source-interleave-x86_64.ll to X86/source-interleave-x86_64.test). Also, my patch creation process may sometimes mess things up (sometimes it thinks things are modified, not added for example), but the final commit will actually be a rename.
> > 
> > If you're referring to renaming "source-interleave-x86_64.c" to something else, then no, because that needs to point at the .c file that this .ll was generated from (and that is unchanged).
> Oh I thought you renamed `source-interleave-x86_64.c` to `source-interleave.c`. If the C source hasn't been renamed, why do you rename the `.ll` file?
I can name it source-interleave-x86_64.ll if you prefer. It's in a different directory to where the code comes from, so it's still a new file.


================
Comment at: test/tools/llvm-objdump/X86/Inputs/source-interleave.ll:17
+define i32 @main() #0 !dbg !14 {
+entry:
+  %retval = alloca i32, align 4
----------------
MaskRay wrote:
> I don't know how you feel about it, but I think these functions can just be empty. That is sufficient to emit debug info entries which will be used by the line table parser.
I'm okay doing this, but maybe in a separate patch, since that keeps the diffs cleaner, and shouldn't impact the new tests. By the way, the original contributor was specifically asked to produce the .ll from the .c file for the original test.


================
Comment at: test/tools/llvm-objdump/X86/Inputs/source-interleave.ll:41
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 4.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, globals: !3)
+!1 = !DIFile(filename: "source-interleave-x86_64.c", directory: "SRC_COMPDIR")
+!2 = !{}
----------------
rupprecht wrote:
> Mind adding a comment here (or maybe at the top) that tests using this file need to run sed to fix this? Took me longer than I would like to admit to realize that.
I've added the comment to the top of the file.


================
Comment at: test/tools/llvm-objdump/X86/source-interleave-invalid-source.test:5
+# RUN: sed -e "s,SRC_COMPDIR,%/p/Inputs,g" %p/Inputs/source-interleave.ll > %t.ll
+# RUN: sed -e "s,line: 7,line: 9999,g" %t.ll > %t2.ll
+
----------------
jhenderson wrote:
> grimar wrote:
> > May be silly question, but what if it is lower? I.e. `s,line: 7,line: -1,g`?
> > Does it makes sence to test it?
> I don't think so. That sounds like it's testing llc's handling of weird looking data. I'll experiment though to see what it actually ends up as.
As I suspected, llc emits an error if passed a negative number.


================
Comment at: test/tools/llvm-objdump/X86/source-interleave-missing-source.test:5
+# RUN: sed -e "s,SRC_COMPDIR,%t,g" %p/Inputs/source-interleave.ll > %t.ll
+# RUN: sed -e "s,SRC_COMPDIR,%/p/Inputs,g" %p/Inputs/source-interleave.ll > %t2.ll
+
----------------
MaskRay wrote:
> jhenderson wrote:
> > MaskRay wrote:
> > > I wanted to ask whether `sed -e 's,SRC_COMPDIR,%p,'` works on Windows, if it works, we don't need to use `%/p`.
> > > 
> > > In a *nix shell, `printf %s '\a\b'` => `\a\b`
> > %p doesn't work in that string, because it becomes something like "C:\test\directory" and the \t and \d get interpreted as escape characters. See also https://reviews.llvm.org/D61856.
> > 
> > Note: I do most of my development on Windows, so I'll pick up issues like that.
> That test said \t didn't work in double quotes. If \t doesn't work in single quotes, either, %/p seems fine.
I've tried single-quotes, and it doesn't work unfortunately.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61996/new/

https://reviews.llvm.org/D61996