[PATCH] D103250: [llvm-dwarfdump][test] Add missing dedicated tests for some options

Owen Reynolds via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri May 28 08:15:19 PDT 2021


gbreynoo marked 4 inline comments as done.
gbreynoo added inline comments.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/parent_recurse_depth.s:22
+
+.section        .debug_abbrev,"", at progbits
+.Lsection_abbrev:
----------------
Higuoxing wrote:
> I think the following yaml code is more readable than raw assembly code. What do you think?
> 
> ```
> --- !ELF
> FileHeader:
>   Class:   ELFCLASS64
>   Data:    ELFDATA2LSB
>   Type:    ET_EXEC
>   Machine: EM_X86_64
> DWARF:
>   debug_abbrev:
>     - Table:
>       - Tag:      DW_TAG_compile_unit
>         Children: DW_CHILDREN_yes
>         Attributes:
>           - Attribute: DW_AT_producer
>             Form:      DW_FORM_string
>       - Tag:      DW_TAG_subprogram
>         Children: DW_CHILDREN_yes
>         Attributes:
>           - Attribute: DW_AT_name
>             Form:      DW_FORM_string
>       - Tag:      DW_TAG_namespace
>         Children: DW_CHILDREN_yes
>         Attributes:
>           - Attribute: DW_AT_name
>             Form:      DW_FORM_string
>       - Tag:      DW_TAG_base_type
>         Children: DW_CHILDREN_no
>         Attributes:
>           - Attribute: DW_AT_name
>             Form:      DW_FORM_string
>   debug_info:
>     - Version: 4
>       Entries:
>         - AbbrCode: 1
>           Values:
>             - CStr: by_hand
>         - AbbrCode: 2
>           Values:
>             - CStr: main
>         - AbbrCode: 3
>           Values:
>             - CStr: test
>         - AbbrCode: 4
>           Values:
>             - CStr: int
> ```
Much better thanks.


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:638
   ToolOutputFile OutputFile(OutputFilename, EC, sys::fs::OF_TextWithCRLF);
-  error("Unable to open output file" + OutputFilename, EC);
+  error("Unable to open output file " + OutputFilename, EC);
   // Don't remove output file if we exit with an error.
----------------
Higuoxing wrote:
> jhenderson wrote:
> > Whilst you're changing this message, I'd also change `Unable` to `unable`, in line with the LLVM error message style policy.
> I think it might be good to surround file names with single quotes `'`. e.g.
> 
> ```
> Unable to open output file '/path/to/file': permission denied
> ```
I'd have to check if OutputFilename is empty or an error like `unable to open output file  '': no such file or directory` can be output which looks strange IMHO . Would it better to leave it as is?


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

https://reviews.llvm.org/D103250



More information about the llvm-commits mailing list