[PATCH] D89216: [dsymutil] Add the ability to run the DWARF verifier on the input

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:275-276
 
+  /// Verify the input DWARF.
+  void setVerify(bool Verify) { Options.Verify = Verify; }
+
----------------
It looks useful to explicitly show that library verifies input DWARF.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:787-788
 
+    /// Verify DWARF
+    bool Verify = false;
+
----------------
It looks useful to explicitly show that library verifies input DWARF.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:2643
+  if (!File.Dwarf->verify(llvm::outs(), DumpOpts.noImplicitRecursion())) {
+    WithColor::warning() << "input verification failed for " << File.FileName
+                         << '\n';
----------------
It is probably better to use DWARFLinker::reportWarning method instead of direct call to WithColor::warning().


================
Comment at: llvm/test/tools/dsymutil/X86/verify.test:18
+# RUN: not dsymutil -verify-dwarf=all -oso-prepend-path=%p/../Inputs -y %s -o %t 2>&1 | FileCheck %s --check-prefixes=QUIET-OUTPUT-FAIL,QUIET-INPUT-FAIL,VERBOSE-INPUT-FAIL
+
+# VERBOSE-INPUT-FAIL: error: Abbreviation declaration contains multiple DW_AT_language attributes.
----------------
it would be good to test -verify-dwarf=none and -verify-dwarf=bad_value.


================
Comment at: llvm/tools/dsymutil/LinkUtils.h:33-34
 
+  /// Verify DWARF
+  bool Verify = false;
+
----------------



================
Comment at: llvm/tools/dsymutil/dsymutil.cpp:88
 
+enum class DWARFVerify : unsigned {
+  None = 0,
----------------
it is not expected to have a lot of values here. Probably, uint8_t instead of unsigned would be better.


================
Comment at: llvm/tools/dsymutil/dsymutil.cpp:91
+  Input = 1 << 0,
+  Output = 1 << 1,
+};
----------------
```
All = Input | Output, 
```

may be useful later in getVerifyKind().


================
Comment at: llvm/tools/dsymutil/dsymutil.cpp:273
+  Options.LinkOpts.Verify =
+      static_cast<bool>(Options.Verify & DWARFVerify::Input);
   Options.LinkOpts.NoOutput = Args.hasArg(OPT_no_output);
----------------
```
static_cast<bool>(Options.Verify & DWARFVerify::Input);
```

looks overweight. Probably implement a helper ?:


```
inline bool FlagIsSet (DWARFVerify Flags, DWARFVerify SingleFlag) {
  return static_cast<uint8_t>(Flags) & static_cast<uint8_t>(SingleFlag);
}
```


```
Options.LinkOpts.Verify = FlagIsSet(Options.Verify, DWARFVerify::Input);
```


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

https://reviews.llvm.org/D89216