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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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)
----------------
grimar wrote:
> 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.
I think removing the check reduces complexity at no real cost (the check goes as does the corresponding testing, perhaps with one new test case). I think it is more obvious if testing things that aren't to do with the section content (like the readobj test that has been modified) to not have to specify `Content: ""`


================
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:
> 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.
It's to reduce reliance on knowledge of the code in the test/make the test less fragile. If somebody tried to refactor the code in the future to check at different points, case 2 might fail due to reordering of checks, even though I think it would be okay to do that check first. Thinking about it, I'd just change case 2 so that it couldn't possibly fail due to the second check (i.e. it has exactly one null byte in it), like my suggestion above.


================
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]);
----------------
grimar wrote:
> 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)
I agree that it seems unnecessary to add the complexity (compiler optimizers should be able to handle it, and we're not in performance critical code anyway). But it is the policy. Maybe suggest a style amendment on the mailing list?


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

https://reviews.llvm.org/D69895