[PATCH] D149095: [llvm-objdump] Support CHPE code ranges in disassembler.

[Jacek Caban via Phabricator via llvm-commits]

jacek marked 5 inline comments as done.
jacek added a comment.

Thanks for reviews.



================
Comment at: llvm/include/llvm/Object/COFF.h:748
 
+enum chpe_range_type { CHPE_RANGE_ARM64, CHPE_RANGE_ARM64EC, CHPE_RANGE_AMD64 };
+
----------------
jhenderson wrote:
> You should explicitly assign these values, as their value is significant, it seems. Also, unless these names are spec-defined (I'm guessing they might be), they should follow the LLVM coding standards for enums (see https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).
This will require changing llvm-readobj, so I moved COFF.h changes to D156797.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1126
+  if (It == MappingSymbols.begin() ||
+      ((--It)->first < SectionAddress && islower(It->second)))
     return '\x00';
----------------
efriedma wrote:
> jacek wrote:
> > MaskRay wrote:
> > > Why is this `islower` change?
> > CHPE ranges, marked with upper letters, can cross section section boundaries. I will add a comment to clarify that.
> Please don't use ctype.h routines; they can be locale-sensitive in some cases, which can lead to weird results.  StringExtras.h has alternative routines you can use.  (It looks like there isn't an islower replacement, but feel free to add it if you need it.)
I created D156796 with new helpers and will use it here.


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

https://reviews.llvm.org/D149095