[PATCH] D116787: [llvm-readobj][MachO] Add option to sort the symbol table before dumping (MachO only, for now).

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D116787#3345634 <https://reviews.llvm.org/D116787#3345634>, @oontvoo wrote:

> I'm not sure that's much less code - the only substantial chunk of code right now is in MachOODumper.cpp, line ~602 to 722.
> The rest is just one or two line of plumping the args through.

My suggested code has about 55 LOC in total, compared to over 70 in this patch just for the comparators as things stand, without even considering the other code. It's also conceptually simpler: simply store a list of functions to sort by upfront, which are akin to `std::less`, and therefore don't need "equals" comparison functions, and then use them directly in the sorter. This is before you consider other complications, such as the use of std::tuple and constants to look up in said tuple, or the need for enums in the argument parsing.



================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:680
+  if (SortKeys && !SortKeys->empty()) {
+    // The references returned by calling Obj->symbols() are temporary,
+    // and we can't hold onto them after the loop. In order to sort the
----------------
oontvoo wrote:
> jhenderson wrote:
> > Are you sure about this? They don't look all that temporary (as long as the object file is still in memory)...
> > Are you sure about this? They don't look all that temporary (as long as the object file is still in memory)...
> 
> Yes - I've run the code with asan(previous version which simply stored the returned values from `->symbols()` to a vector then sorted it) and got errors
Are you sure it was that bit of code causing the problem? I've inspected the implementation of `symbols()` and `SymbolRef` for Mach-O, and the `SymbolRef` in this case is just a pointer into the buffer stored by MachOObjectFile (see `MachOObjectFile::getSymbolByIndex`). As such, there should be no lifetime issues, as long as the object file is in existence. Please try again and analyse where the issue actually is, as it could be a bug elsewhere that this just exposed. I'm going to need more convincing than just "asan said so" that there's an issue storing a SymbolRef in a vector that has a lifetime less than that of the object file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116787