[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/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:
> > jhenderson wrote:
> > > 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.
> > > 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
> > 
> > I am not sure what do you mean by "fail". These test fall back to emiting the "Content". Reordering of them should emit the "Content" still.
> > 
> > > 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.
> > 
> > Having a content that is "00" instead of "61" does not trigger the first `if`, so it breaks the current test actually. I'd suggest to check the current logic and not the logic we might have or not have in case if somebody decide to break it in the future... :)
> > (I have no preblems to add a one more test for "00" if you want though. Should I?"
> > 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)
> 
> I think I see the reason of misunderstanding now. I have cases numbered 1, 2 and 2, so I was talking about the second one, while you seems suggested to change the third, which just has a wrong number. I'll change it from '6100' to '00'.
Sorry, I meant falling back to Content when I said "fail". ("fail" as in "failed to parse").

Actually, I think I may have been talking about the real case 2 (i.e. the second one). I'm getting myself a bit confused though.

I think it makes sense to change both cases to not fail the other check, so "610062" would fail the null-termination check (but not the odd pairs check), whilst "6100" fails the pairs check, but not the null-termination.

Another thought, prompted by the suggestion of the "00" case - should it be an error to have an empty string in either parameter? E.g. Should "0000"/"610000"/"006100" fail?

Also, I'm not sure it's obvious that "00" represents an incomplete pair. Perhaps we should just talk about empty strings instead? A split on "\0" would usually render a pair of {"", ""} if run on a string containing a single '\0'.


================
Comment at: llvm/test/tools/yaml2obj/linker-options.yaml:104
+
+## Check we can omit both "Options" and "Content". Then we produce an empty section.
+
----------------
Perhaps "The we produce" -> "This produces"


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

https://reviews.llvm.org/D69895