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

[George Rimar via Phabricator via llvm-commits]

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())))
----------------
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).





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

https://reviews.llvm.org/D68983