[PATCH] D115418: [dwarf][NFC] Move expandBundle() to MachO.h

Greg Clayton via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 9 16:15:39 PST 2021


clayborg added inline comments.


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:4725-4727
+Expected<bool>
+MachOObjectFile::isDsymBundle(const StringRef Path,
+                              std::vector<std::string> &BinaryPaths) {
----------------
My question here is: if an error is returned, will it make any sense to the caller? The only error returned here is if something goes wrong during directory iteration. Which could be a few things:
- Path specifies a directory with ".dSYM" extension (do we are about case here?) and it doesn't the directory Path + "Context/Resources/DWARF" doesn't exist which would return something like "directory does not exist". Will it specify which directory doesn't exist? The user could specify "/path/to/foo.dSYM" that is a valid directory but it contains no DWARF bundle stuff inside. Just wondering if this will make sense to show to the user?
- We can't get sys::fs::file_status on a file in the Path + "Context/Resources/DWARF" + File file which would return something like "permission denied" or something else. If the error doesn't contain the path that caused the problem the user will be confused.

And I wonder if the callers of this function should show those errors to the user and would it make any sense to the user? If the errors don't make sense, then it might be good to just return a "bool" instead of a "Expected<bool>", or just return a std::vector<std::string> that is empty or that contained any successful paths that were already extracted.

If we want to keep the errors we should make the error message more clear and include information about what path caused the problem with the EC error code appended. So for the first case where sys::fs::directory_iterator constructor fills in an error, maybe returning something like "unable to iterate over directory %s: %s" where the first %s is the Path and the second is the error string. Something similar for the second?

Let me know what your thoughts are.


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:651-655
+    auto IsDsymOrErr = MachOObjectFile::isDsymBundle(F, Objects);
+    if (auto Err = IsDsymOrErr.takeError())
+      error(F, std::move(Err));
+    if (!*IsDsymOrErr)
+      Objects.push_back(F);
----------------
Avoid having to take the error and test it by restructuring the code.


================
Comment at: llvm/tools/llvm-gsymutil/llvm-gsymutil.cpp:389-393
+  auto IsDsymOrErr = MachOObjectFile::isDsymBundle(ConvertFilename, Objects);
+  if (auto Err = IsDsymOrErr.takeError())
+    error(ConvertFilename, std::move(Err));
+  if (!*IsDsymOrErr)
+    Objects.push_back(ConvertFilename);
----------------
Avoid having to take the error and test it by restructuring the code.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115418



More information about the llvm-commits mailing list