[PATCH] D40057: Simplify file handling in dsymutil

Jonas Devlieghere via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Nov 15 02:52:56 PST 2017


JDevlieghere added a comment.

Thanks Rafael! I added the `ToolOutputFile` when implementing the threading option because I wanted to make sure files were removed if some thread were to exist dsymutil. Because we only use threads when there are multiple architectures which in turn  implies we use temp files, we would indeed end up removing the file twice.

It's definitely an improvement to keep the file handling logic out the DwarfLinker. I'm not really happy with how we deal with files in `dsymutil.cpp` though and maybe this is the opportunity to improve it. For one, I don't like that `getOutputFileName` is doing so much more than just generating a file name. I also don't like that we don't remove non-temporary output files if linking fails.

Maybe we can start by splitting up the logic for temporary files (using `TempFile`) and output files (using `ToolOutputFile`). The former would contribute to your goal of unifying the use of `TempFile` and the latter would address my second concern. Things are a little tricky for dSYM bundles, where we ideally would want to remove the whole bundle rather than just the file. There's also the call to `generateUniversalBinary` which takes a file path rather than a handle.

I don't know if this is the best approach and how feasible it is, but consider this a way to kick off the discussion!


https://reviews.llvm.org/D40057





More information about the llvm-commits mailing list