[PATCH] D86539: [Debuginfo][llvm-dwarfutil] llvm-dwarfutil dsymutil-like tool for ELF.

Thu Apr 7 08:43:28 PDT 2022

avl added inline comments.

================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/copy-itself.test:12-27
+# RUN: yaml2obj %s -o %t1
+# RUN: llvm-dwarfutil %t1 --separate-debug-file %t1
+# RUN: llvm-objdump --headers %t1 | FileCheck -check-prefix=CHECK-NON-DEBUG %s
+# RUN: llvm-dwarfdump -a %t1.debug | FileCheck -check-prefix=CHECK-DEBUG %s
+
+# RUN: yaml2obj %s -o %t1
+# RUN: llvm-dwarfutil --no-garbage-collection %t1 %t1
----------------
jhenderson wrote:
> avl wrote:
> > jhenderson wrote:
> > > As stated in my previous comment, you don't need these test cases: they duplicate coverage provided elsewhere. There's no need to test that the copy-self behaviour works with every option, because the behaviour is unrelated to those options.
> > It actually does not duplicate coverage. Using combination of these options(--garbage-collection, --no-garbage-collection, --separate-debug-file) leads to crossing different code paths:
> > 
> > llvm-dwwarfutil.cpp:219
> > llvm-dwwarfutil.cpp:244
> > llvm-dwwarfutil.cpp:331
> > llvm-dwwarfutil.cpp:359
> > llvm-dwwarfutil.cpp:395
> > 
> > Thus it looks good if all of them would be covered.
> I'm not sure what you mean by "crossing different code paths" here exactly, but `writeToOutput`'s behaviour is already tested, and a single test case is sufficient to show that writing to the input file via this method is sufficient, assuming that code calling that function doesn't do anything odd like open the input file again etc. Given that it doesn't currently, a test isn't needed until someone adds that functionality. Your non self-copy tests are sufficient to show that output is written correctly when those options are used.
If the implementation would be changed later - and instead of writeToOutput there would be used another code in some places - then we will not have a check that proves everything is working as expected(if we will remove these tests).  

================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/copy.test:16
+
+## Check that debug info is stored in separate file.
+# RUN: llvm-dwarfutil --no-garbage-collection %t.o --separate-debug-file %t3
----------------
jhenderson wrote:
> Shouldn't these cases be in a dedicated test file for `--separate-debug-file`?
>From my point of view there is no difference whether we have dedicated test file for --separate-debug-file or it is tested inside other test cases. If you think we need one - I will do it.

================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/error-corrupted-input-file.test:6
+
+# CHECK: archive.a: error: corrupted input file
----------------
jhenderson wrote:
> The format would usually be `error: 'archive.a': corrupted input file`. See `createFileError` (which you should be using).
> 
> That being said, how is this input file "corrupt"? The file itself is a perfectly valid thin archive file. Shouldn't this just be something like "unsupported file format" or similar?
The same error might occur if the source file is corrupted. Possible good message would be :

error: 'archive.a': unsupported or corrupted input file

?

================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/verbose.test:10
+## Check warning displayed in multi-thread mode.
+# RUN: llvm-dwarfutil %t.o %t1 --verbose 2>&1 | FileCheck %s --check-prefix=CHECK-THREAD-WARNING
+
----------------
jhenderson wrote:
> I'm not convinced you should have a warning here, because otherwise the --verbose option will always produce a warning unless an explicit --num-threads option is specified. I'm not sure though. Perhaps documenting it in the CommandGuide and help text would be sufficient?
If that warning looks annoying - yes we can avoid it and document the behavior. To check my understanding - the warning should not be shown in all cases: in default "llvm-dwarfutil %t.o %t1 --verbose"  and in the explicit : "llvm-dwarfutil %t.o %t1 --verbose --num-threads 10", right ?

================
Comment at: llvm/test/tools/llvm-dwarfutil/error-invalid-format.test:8
+
+# CHECK: error: The file was not recognized as a valid object file
----------------
jhenderson wrote:
> jhenderson wrote:
> > It would be nice if this had the file's name in the message.
> Not addressed?
it looks addressed. error-invalid-format.test - is the file name. It probably should be in quotas(using createFileError) like you said in other comment.

================
Comment at: llvm/test/tools/llvm-dwarfutil/error-no-input-file.test:6
+
+# CHECK: non-existed: error: No such file or directory
----------------
jhenderson wrote:
> jhenderson wrote:
> > It would be nice if this had the file's name in the message.
> Not addressed.
> 
> Also, this needs to use a lit substitution for the message format. See the %errc* substitutions.
not-existed - is the file name.

================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:174
+    if (ObjectFile *Obj = static_cast<ObjectFile *>(BinOrErr->getBinary())) {
+      verbose("Verifying DWARF...", Opts.Verbose);
+      std::unique_ptr<DWARFContext> DICtx = DWARFContext::create(*Obj);
----------------
jhenderson wrote:
> avl wrote:
> > jhenderson wrote:
> > > Test case?
> > It is quite tricky to create a test case here. Changing this to asserts does not look right. I think it is safe to leave it as is.
> Looks like you made a test case after all :-)
no. there is no test case for it still.

================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:332
+  if (Error Err = writeToOutput(
+          Config.Common.OutputFilename, [&](raw_ostream &OutFile) -> Error {
+            raw_crc_ostream CRCBuffer(OutFile);
----------------
jhenderson wrote:
> avl wrote:
> > jhenderson wrote:
> > > Is the trailing return type needed?
> > I think it is always useful to type it explicitly. It clearly shows return type. Otherwise you need to compile the lambda by itself.  If it is against of guideline - then would remove it.
> > Otherwise you need to compile the lambda by itself
> 
> I don't understand this comment.
> 
> > If it is against of guideline - then would remove it.
> 
> It's because it's superfluous noise - I'm not sure there's a specific coding standard on it though. Since this lambda is being passed directly into a function, you don't need to know what its return type is due to how you use it later. Within the function, it's clear that the return type is `Error`, so it doesn't really help with readability of the function either, in my opinion. Consequently, the extra code means it's harder for me to read everything.
> > Otherwise you need to compile the lambda by itself
> 
> I don't understand this comment.

I mean that the type of returning value is not clear while someone inspecting the code. Someone need to check types of expressions in the "return" clauses, type of argument where lambda is used, etc...

> 
> > If it is against of guideline - then would remove it.
> 
> It's because it's superfluous noise - I'm not sure there's a specific coding standard on it though. Since this lambda is being passed directly into a function, you don't need to know what its return type is due to how you use it later. Within the function, it's clear that the return type is `Error`, so it doesn't really help with readability of the function either, in my opinion. Consequently, the extra code means it's harder for me to read everything.

My understanding is opposite. It is not really clear whether return type is Error, or instead it is Expected<something> or something else.. If returning type is hidden it requires less letters but more actions to understand.

so I think the benefit of using explicit return value is more than created noise.

================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:416-417
+    } else
+      return createStringError(std::errc::invalid_argument,
+                               "possible broken debug info");
+
----------------
jhenderson wrote:
> jhenderson wrote:
> > Test case?
> Not addressed?
yep, it is not addressed yet. Returning type of linkDebugInfo is kind of artificial at the moment. I will change it in followup review. Currently it is hard to create a testcase when it returns false.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86539