[PATCH] D69895: [yaml2obj/obj2yaml] - Add support for SHT_LLVM_LINKER_OPTIONS sections.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:332
+struct LinkerOption {
+  LinkerOption(StringRef K, StringRef V) : Key(K), Value(V) {}
+  LinkerOption() = default;
----------------
MaskRay wrote:
> Just delete user-defined constructors.
I've added them to use `S->Options->emplace_back(Strings[I], Strings[I + 1]);` (In obj2yaml, otherwise it won't compile).
Perhaps it is too much and so I've removed them and used `S->Options->push_back({Strings[I], Strings[I + 1]});` instead.
I have no strong feeling here, perhaps both approaches are OK, it is not a place that requires hard optimisations.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1353-1354
+          dyn_cast<ELFYAML::LinkerOptionsSection>(Section.get())) {
+    if (!Sec->Options && !Sec->Content)
+      return "either \"Options\" or \"Content\" must be specified";
+    if (Sec->Options && Sec->Content)
----------------
MaskRay wrote:
> jhenderson wrote:
> > Hmm... I think it should be okay to not have to specify anything to create an empty such section (I'd actually support this for other sections too, thinking about it, where an empty section is legitimate).
> Deleting the check here looks good to me.
I think having an empty linker options section is uncommon? We always can use `Content=""` to explicitly show our intention to create such section. Is it a problem to require a `Content` then?
To clarify: I am OK to change it, but probably see no reason to add a complexity here.


================
Comment at: llvm/test/tools/obj2yaml/linker-options.yaml:61
+    Content: ""
+## 2) Non-null terminated content.
+  - Name: .linker-options-no-null
----------------
MaskRay wrote:
> jhenderson wrote:
> > Perhaps also a case combining 2) and 3), i.e. Content: "610062" or something like that.
> Non-NUL terminated
I see why you want to use `NUL`, but I am not sure is this place really wrong? wiki says:
"In computer programming, a null-terminated string is a character string stored as an array containing the characters and terminated with a null character ('\0', called NUL in ASCII)."
(https://en.wikipedia.org/wiki/Null-terminated_string)

I see that "non-null terminated string" is used widely (just google it). (And "non-NUL" looks a bit uncommon to me and seems almost never used in the wild). Should I change it?


================
Comment at: llvm/test/tools/obj2yaml/linker-options.yaml:61-64
+## 2) Non-null terminated content.
+  - Name: .linker-options-no-null
+    Type: SHT_LLVM_LINKER_OPTIONS
+    Content: "61"
----------------
grimar wrote:
> MaskRay wrote:
> > jhenderson wrote:
> > > Perhaps also a case combining 2) and 3), i.e. Content: "610062" or something like that.
> > Non-NUL terminated
> I see why you want to use `NUL`, but I am not sure is this place really wrong? wiki says:
> "In computer programming, a null-terminated string is a character string stored as an array containing the characters and terminated with a null character ('\0', called NUL in ASCII)."
> (https://en.wikipedia.org/wiki/Null-terminated_string)
> 
> I see that "non-null terminated string" is used widely (just google it). (And "non-NUL" looks a bit uncommon to me and seems almost never used in the wild). Should I change it?
Just to clarify. The current code is:

```
  ArrayRef<uint8_t> Content = *ContentOrErr;
  if (Content.empty() || Content.back() != 0) {
    S->Content = Content;
    return S.release();
  }

  SmallVector<StringRef, 16> Strings;
  toStringRef(Content.drop_back()).split(Strings, '\0');
  if (Strings.size() % 2 != 0) {
    S->Content = Content;
    return S.release();
  }
```

I.e. "610062" will be captured by the first if, just like "61". What do you want to check with "610061"?
At least I need to know the intention to write a comment :) Perhaps we should not do that.


================
Comment at: llvm/test/tools/yaml2obj/linker-options.yaml:7
+# RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-readobj --sections --section-data %t1 | FileCheck %s --check-prefix=OPTIONS
+
----------------
jhenderson wrote:
> Rather than the --section-data switch, it might be clearer to use the --string-dump switch to dump the first section. That way, you can more easily match the original strings to their contents. (I assume you're deliberately not using the --elf-linker-options switch)
`--string-dump` looks fine here, thanks, I've forgot about it.

> I assume you're deliberately not using the --elf-linker-options switch
Yes, I wanted to use an independent approach to test the bytes emmitted.


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:626
+  S->Options = std::vector<ELFYAML::LinkerOption>();
+  for (size_t I = 0; I != Strings.size(); I += 2)
+    S->Options->emplace_back(Strings[I], Strings[I + 1]);
----------------
jhenderson wrote:
> Isn't LLVM style to calculate the Strings size only once?
> 
> `for (size_t I = 0, E = Strings.size(); I != E; I += 2)`
Ok. I do not understand this rule though: `size()` returns a constant, I see no reasons to hard-think about it. It probably comes from the past, when
"In C++11 it is required that for any standard container the .size() operation must be complete in "constant" complexity (O(1))", but "Previously in C++03 .size() should have constant complexity, but is not required". (https://stackoverflow.com/questions/228908/is-listsize-really-on/13751799#13751799)


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

https://reviews.llvm.org/D69895