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

Fri Apr 8 03:40:28 PDT 2022

jhenderson 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
----------------
avl wrote:
> jhenderson wrote:
> > avl wrote:
> > > 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).  
> > And at that point, it would be up to the developer to consider the new behaviour and how it needs testing though. Regardless, that code will still be tested by basic testing for each of the different options - the combination of them doesn't change the fact that it gets called ultimately. Combining the options doesn't change the code path of the area under test to something that can't be hit by it. If we followed your logic, we'd write tests for every possible combination of tool options for every tool, which is clearly impractical.
> not every possible but every relevant. We have several paths which could lead to writing to the file - it would be good if all of these paths would be covered by test. 
> 
> But if we want to reduce that level of testing - I will left only one case.
To be clear, I agree that all call sites for writeToOutput need testing (i.e. the ones you've highlighted above). But I don't think they need testing for the "copy self" case, because that's not significantly different to the "copy to other file" case, as long as there is a test that shows "copy self" in general works.

================
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
----------------
avl wrote:
> 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.
Normally, I'd have a dedicated test file per option, with possible additional ones for testing option interactions where needed. `--[no-]separate-debug-file` is a distinct option, hence I think it should be tested in a separate file, rather than mixing multiple unrelated test axes in one file.

================
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);
----------------
avl wrote:
> jhenderson wrote:
> > avl wrote:
> > > 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.
> > `verbose.test` line 24 checks "Verifying DWARF..." though?
> my comment was about 181 line :
> 
> return createError("unsupported file: '" + FileName + "'");
> 
> this line is not covered by test and it is tricky to do it.
Digging into the code some more, the "unsupported file" case can only be hit if the created Binary instance is not an object, but is a recognised Binary type. For this to happen, the output file would have to have been written in an invalid state, no? That seems like an internal error within the program, so possibly should be an llvm_unreachable or similar?

================
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);
----------------
avl wrote:
> jhenderson wrote:
> > avl wrote:
> > > 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.
> > > 
> > > 
> > I've created a Discourse thread (https://discourse.llvm.org/t/coding-standard-for-lambda-trailing-return-types/61569) to discuss the general principle, because I don't agree with you, and I think it's clear we need a general standard on this topic.
> > 
> > > 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...
> > 
> > In this particular context, when does somebody need to know the return type? How do `return Err;` and `return Error::success();` not express the return type clearly?
> > 
> > > My understanding is opposite. It is not really clear whether return type is Error, or instead it is Expected<something> or something else.
> > 
> > The use of `Error::success()` rules out `Expected<T>` as a return type. Instead it would return a concrete value. Again though, my question is, why do you need to know the return type in this context?
> > 
> > I argue the following for this specific case:
> > 1. The lambda is being passed directly into a function so we don't have to worry about capturing it in a variable and ensuring the variable's type matches that of the lambda return value.
> > 2. The function is small. If someone needs to modify it, they'll need to understand what the function does anyway, and it's obvious from what is being returned: the use of `Err` as a variable name, immediately after assigning it to an `Error` variable, combined with `Error::success()`.
> > 3. The compiler will tell you if you get something wrong when modifying the code (either the function signature changing or somebody modifying the lambda). If developing in an IDE, this error may even appear as you are typing, without the need to explicitly write the compiler.
> > I've created a Discourse thread (https://discourse.llvm.org/t/coding-standard-for-lambda-trailing-return-types/61569) to discuss the general principle, because I don't agree with you, and I think it's clear we need a general standard on this topic.
> > 
> Thanks. I will join to this thread.
> 
> > > 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...
> > 
> > In this particular context, when does somebody need to know the return type? How do `return Err;` and `return Error::success();` not express the return type clearly?
> > 
> 
> In this particular context it is more or less obvious. But in general it is very subjective - whether it is obvious or not. C++ is very complex language. If you see return true - it does not mean return value is bool. if you see return foo() - it is not seen which type it is. I think it is easier to just have a return type than think whether it is obvious or not.
> 
> > > My understanding is opposite. It is not really clear whether return type is Error, or instead it is Expected<something> or something else.
> > 
> > The use of `Error::success()` rules out `Expected<T>` as a return type. Instead it would return a concrete value. Again though, my question is, why do you need to know the return type in this context?
> 
> In this particular context it is more or less obvious(Though it still requires to check several places(return Err, Error Err, return Error::success()) instead of one(-> Error)). The thing which does not worth a time, from my opinion -> thinking whether it is enough obvious or not.
> 
> > 
> > I argue the following for this specific case:
> > 1. The lambda is being passed directly into a function so we don't have to worry about capturing it in a variable and ensuring the variable's type matches that of the lambda return value.
> > 2. The function is small. If someone needs to modify it, they'll need to understand what the function does anyway, and it's obvious from what is being returned: the use of `Err` as a variable name, immediately after assigning it to an `Error` variable, combined with `Error::success()`.
> > 3. The compiler will tell you if you get something wrong when modifying the code (either the function signature changing or somebody modifying the lambda). If developing in an IDE, this error may even appear as you are typing, without the need to explicitly write the compiler.
> 
> My main point is that it would be easier to not waste a time on thinking whether each concrete case is obvious enough or not. 
My point is less if it's obvious and more whether it matters. In my experience, in 99% of cases, it doesn't with regards to lambdas because of how they are used. Anyway, let's see how that discussion in Discourse pans out. I'm not opposed to trailing return types in some cases where it really isn't obvious and it does matter, but I don't believe that to be the case here. It sounds to me like you're advocating for "always have return types".

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86539