[PATCH] D86539: [Debuginfo][llvm-dwarfutil] llvm-dwarfutil dsymutil-like tool for ELF.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 6 00:44:03 PDT 2022
jhenderson added inline comments.
================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinkerCompileUnit.h:187
+ /// overlaps with existing function ranges.
+ bool overlapsWithFunctionsRanges(uint64_t LowPC, uint64_t HighPC);
+
----------------
================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:504-505
+ if (Unit.overlapsWithFunctionsRanges(*LowPc, *HighPc)) {
+ reportWarning(formatv("Overlapped address range [{0:X}, {1:X}]. Range will "
+ "be discarded.\n",
+ *LowPc, *HighPc),
----------------
Noting that this warning message (and indeed others in this file) don't conform to the current LLVM style guide. It also seems like `\n` should be part of the `reportWarning` function itself. I'd certainly expect it to be.
================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/copy-itself.test:7
+# RUN: llvm-dwarfutil %t1 %t1
+# RUN: llvm-dwarfdump -a %t1 | FileCheck -check-prefix=CHECK-DEBUG %s
+# RUN: llvm-dwarfutil %t1 %t2
----------------
Nit: here and throughout, I'd suggest using `--check-prefix` (two dashes), since it is the more common format for long options. It's also odd having some test cases using double dash for one command and then single dash for the FileCheck command.
================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/copy-itself.test:8-10
+# RUN: llvm-dwarfutil %t1 %t2
+# RUN: llvm-dwarfdump -a %t2 | FileCheck -check-prefix=CHECK-DEBUG %s
+# RUN: diff %t1 %t2
----------------
When I suggested adding this test case, I meant you should have it somewhere, but it probably doesn't belong in this test file. It might be more appropriate in the "standard" copy test file for this tool.
================
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
----------------
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.
================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/copy.test:1-2
+## This test checks that debug info contained in the source file
+## is fully copied to the destination file.
+
----------------
It seems like `gc-default.test` covers much of this test's behaviour? Do we need both?
`--separate-debug-file` should probably be tested in its own test file.
================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/copy.test:17-18
+
+# RUN: llvm-dwarfutil %t.o --separate-debug-file --no-separate-debug-file %t1
+# RUN: llvm-dwarfdump -a %t1 | FileCheck -check-prefix=CHECK-DEBUG %s
+
----------------
This doesn't show that `--no-separate-debug-file` is the option being processed... If you removed that option the test would still pass!
================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/gc-bfd.test:1
+## This test checks that debug info related to deleted code(marked with
+## tombstone=bfd) is removed.
----------------
In English text, opening parentheses are always preceded by a space (and closing are followed by one). Applies throughout.
================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/gc-func-overlapping-address-ranges.test:1
+## This test checks that overlapped function address ranges
+## are ignored(i.e. left unchanged) by --garbage-collection
----------------
"overlapped" -> "overlapping" here and in the warning message. Same in the compile unit version of this test case.
================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/odr-class.test:11
+
+# RUN: llvm-dwarfutil --odr %t.o - | llvm-dwarfdump -a - | FileCheck %s -check-prefixes=CHECK,CHECK-ODR
+
----------------
Here and in similar tests, it would be useful for a short comment explaining what each individual case is testing, e.g. here it is "show that --odr is the default behaviour".
Additionally, where you are checking an alias behaviour/default behaviour that you've already tested in another way, i.e. where the output should be identical, use `cmp` to compare the previous output against the new output. This removes the additional llvm-dwarfdump and FileCheck runs, and also more rigidly ensures identity.
================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/verify.test:12-14
+# RUN: not llvm-dwarfutil --no-garbage-collection %t.o %t1 --verify
+
+# RUN: not llvm-dwarfutil --no-garbage-collection %t.o --separate-debug-file %t1 --verify
----------------
Check the error messages.
================
Comment at: llvm/test/tools/llvm-dwarfutil/error-invalid-format.test:1-2
+## This test checks that llvm-dwarfutil displaysi the error message
+## if input file of unsupported format was specified.
+
----------------
================
Comment at: llvm/test/tools/llvm-dwarfutil/error-invalid-format.test:4
+
+# RUN: echo 'BAD' > %t.o
+
----------------
You can just use `%s` as the llvm-dwarfutil test input, if you want, to save even needing to do this echo.
================
Comment at: llvm/test/tools/llvm-dwarfutil/error-invalid-format.test:8
+
+# CHECK: error: The file was not recognized as a valid object file
----------------
It would be nice if this had the file's name in the message.
================
Comment at: llvm/test/tools/llvm-dwarfutil/error-no-gc-odr.test:1-2
+## This test checks the error message displayed if an incorrect
+## tombstone value is specified.
+
----------------
Comment does not line up with what the test is doing.
================
Comment at: llvm/test/tools/llvm-dwarfutil/error-no-input-file.test:6
+
+# CHECK: non-existed: error: No such file or directory
----------------
It would be nice if this had the file's name in the message.
================
Comment at: llvm/test/tools/llvm-dwarfutil/error-positional-args.test:7
+
+# CHECK1: error: exactly two positional arguments should be specified but 1 were provided
+# CHECK3: error: exactly two positional arguments should be specified but 3 were provided
----------------
"1 were" is bad English. Perhaps we can avoid additional logic in the error message production by changing to "exactly two positional arguments expected, N provided" which is also a bit shorter too.
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:76-77
+static Error validateAndSetOptions(opt::InputArgList &Args, Options &Options) {
+ for (auto *Arg : Args.filtered(OPT_UNKNOWN))
+ return createError("unknown option: " + Arg->getSpelling());
+
----------------
The loop here is rather superfluous: if `Args.filtered` returns an empty list, no error will be produced, and if it doesn't, the first entry will provoke an error, and not later ones. Perhaps simpler to make it an `if` clause somehow?
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:116-121
+ if (Options.Verbose) {
+ if (Options.NumThreads != 1)
+ warning("--num-threads set to 1 because Verbose mode is specified");
+
+ Options.NumThreads = 1;
+ }
----------------
Test case?
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:118
+ if (Options.NumThreads != 1)
+ warning("--num-threads set to 1 because Verbose mode is specified");
+
----------------
Don't think you need upper-case here.
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:128-133
+ if (Options.BuildSeparateDebugFile && Options.OutputFileName == "-") {
+ warning(
+ "separate file with debug information would not be created. writing "
+ "to stdout");
+ Options.BuildSeparateDebugFile = false;
+ }
----------------
Test case?
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:141
+ // Add new debug sections.
+ for (const SectionRef &Sec : ObjFile.sections()) {
+ Expected<StringRef> SecName = Sec.getName();
----------------
`SectionRef` is designed to be trivially copyable and doesn't really need the `const &` bit.
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:160-163
+ if (Opts.OutputFileName == "-") {
+ warning("verification skipped because writing to stdout");
+ return true;
+ }
----------------
Not tested.
================
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);
----------------
Test case?
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:182
+
+ return createError("unsupported file: '" + FileName + "'");
+}
----------------
Test case?
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:200-207
+public:
+ explicit raw_crc_ostream(raw_ostream &O) : OS(O) { SetUnbuffered(); }
+
+ void reserveExtraSpace(uint64_t ExtraSize) override {
+ OS.reserveExtraSpace(ExtraSize);
+ }
+
----------------
It's more common to have the public interface first in the class, I find.
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:220
+ if (Error Err = writeToOutput(
+ Config.Common.OutputFilename, [&](raw_ostream &OutFile) -> Error {
+ raw_crc_ostream CRCBuffer(OutFile);
----------------
Do you need the trailing return type?
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:245
+ if (Error Err = writeToOutput(
+ Config.Common.OutputFilename, [&](raw_ostream &OutFile) -> Error {
+ if (Error Err =
----------------
Ditto.
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:271
+
+using DebugInfoBits = SmallString<100000>;
+
----------------
Is `SmallString` really the right answer here for such a large base size?
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:293
+ } else if (dyn_cast<ELFObjectFile<ELF32BE>>(&InputFile)) {
+
+ Expected<ELFObjectFile<ELF32BE>> MemFile = ELFObjectFile<ELF32BE>::create(
----------------
Delete this blank line.
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:302
+ } else if (dyn_cast<ELFObjectFile<ELF64BE>>(&InputFile)) {
+
+ Expected<ELFObjectFile<ELF64BE>> MemFile = ELFObjectFile<ELF64BE>::create(
----------------
Ditto.
================
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);
----------------
Is the trailing return type needed?
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:360
+ if (Error Err = writeToOutput(
+ Config.Common.OutputFilename, [&](raw_ostream &OutFile) -> Error {
+ return objcopy::executeObjcopyOnBinary(Config, InputFile, OutFile);
----------------
Ditto.
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:396
+ if (Error Err = writeToOutput(
+ Config.Common.OutputFilename, [&](raw_ostream &OutFile) -> Error {
+ return objcopy::executeObjcopyOnBinary(Config, InputFile, OutFile);
----------------
Ditto.
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:406
+ if (Opts.DoGarbageCollection) {
+ verbose("Do garbage collection for debug info ...", Opts.Verbose);
+
----------------
Test case?
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:416-417
+ } else
+ return createStringError(std::errc::invalid_argument,
+ "possible broken debug info");
+
----------------
Test case?
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:486
+ if (!(*BinOrErr)->isObject()) {
+ error(Opts.InputFileName, "corrupted input file");
+ return EXIT_FAILURE;
----------------
Test case?
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:512
+ if (!*IsCorrect) {
+ error(Opts.InputFileName, "output verification failed");
+ return EXIT_FAILURE;
----------------
Test case?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86539/new/
https://reviews.llvm.org/D86539
More information about the llvm-commits
mailing list