[PATCH] D68983: [yaml2obj, obj2yaml] - Add support for SHT_NOTE sections.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: include/llvm/Support/YAMLTraits.h:652
     return QuotingType::Single;
-  if (isspace(S.front()) || isspace(S.back()))
+  if (isspace(static_cast<unsigned char>(S.front())) ||
+      isspace(static_cast<unsigned char>(S.back())))
----------------
grimar wrote:
> MaskRay wrote:
> > Interesting. Is there a test covering the fixed bug?
> I faced it running the `tools\obj2yaml\missing_symtab.test` initially.
> It uses a precompiled binary.
> I planned to convert it and add a separate test case for this place later,
> but after your comment I decided to do it right now and this revealed
> a different bug:
> 
> I am creating notes in `dumpNoteSection()`.
> I used `toStringRef` before this diff:
> 
> ```
> Entries.push_back({Note.getName(), toStringRef(Note.getDesc()), Note.getType()});
> ```
> 
>  It was there because previously `Desc` was a `StringRef`, but now it is a `yaml::BinaryRef`,
> so `toStringRef` became excessive and actually harmfull.
> 
> Problem of `toStringRef` is that BinaryRef constructor has a different behavior for
> `StringRef` and `ArrayRef<uint8_t>`):
> 
> ```
>   BinaryRef(ArrayRef<uint8_t> Data) : Data(Data), DataIsHexString(false) {}
>   BinaryRef(StringRef Data) : Data(arrayRefFromStringRef(Data)) {}
> 
> ```
> 
> it wasn't correct to use `StringRef` version and I removed this change
> from the patch after the fix, as it is not needed anymore.
> 
> I've added a one more test to `elf-sht-note.yaml` that covers this place.
> 
> I think I might be able to trigger this issue in a different way and going to investigate how
> and suggest an independent patch for this place probably.
> (I'll try to corrupt a section name in the same way, it should work I think).
> 
> 
> 
Patch: D69160


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

https://reviews.llvm.org/D68983