[libcxx-commits] [PATCH] D60416: [libc++] Make sure that the symbol differ takes into account symbol types

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D60416#1461648 <https://reviews.llvm.org/D60416#1461648>, @EricWF wrote:

> So `_symbol_difference` is used to check for added or removed symbols. `changed_symbols` is the bit of logic that detects if the properties of the symbol changed.
>
> What's the issue this is trying to address?


The problem this is trying to address is that we don't take into account the addition of an indirect symbol (a re-export). Try this:

  cat <<EOF > old.sym
  {'is_defined': False, 'name': '___cxa_throw_bad_array_new_length', 'type': 'U'}
  EOF
  
  cat <<EOF > new.sym
  {'is_defined': True, 'name': '___cxa_throw_bad_array_new_length', 'type': 'I'}
  {'is_defined': False, 'name': '___cxa_throw_bad_array_new_length', 'type': 'U'}
  EOF
  
  ./libcxx/utils/sym_diff.py old.sym new.sym

You'll see that despite the fact that we added an indirect symbol, we consider both symbol lists to be the same. This was discovered while working on https://reviews.llvm.org/D60424, which adds such re-exports.

Looking at it again, it looks like the "problem" is that we don't support duplicate symbol names in the list. For example, we use a set in `_symbol_differences`.

Is there a reason for not simply normalizing these list (sorting each line and also sorting each JSON entry), and then simply running diff on it? It seems to me that this would take into account all the "aspects" related to a symbol, automatically.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60416