SHT_LLVM_LINKER_OPTIONS section contains pairs of null-terminated strings.
This patch adds support for them.
Details
Diff Detail
Event Timeline
I assume that this is in preparation for updating a test that uses this section?
llvm/lib/ObjectYAML/ELFYAML.cpp | ||
---|---|---|
1353–1354 | 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). | |
llvm/test/tools/obj2yaml/linker-options.yaml | ||
1 | sections -> section | |
35 | corrupted -> corrupt | |
38 | CORRUPTED -> CORRUPT | |
61–64 | Perhaps also a case combining 2) and 3), i.e. Content: "610062" or something like that. | |
llvm/test/tools/yaml2obj/linker-options.yaml | ||
7 | 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) | |
103 | should -> must (but see my earlier point) | |
llvm/tools/obj2yaml/elf2yaml.cpp | ||
626 | Isn't LLVM style to calculate the Strings size only once? for (size_t I = 0, E = Strings.size(); I != E; I += 2) |
- Addressed review comments.
llvm/include/llvm/ObjectYAML/ELFYAML.h | ||
---|---|---|
332 | I've added them to use S->Options->emplace_back(Strings[I], Strings[I + 1]); (In obj2yaml, otherwise it won't compile). | |
llvm/lib/ObjectYAML/ELFYAML.cpp | ||
1353–1354 | 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? | |
llvm/test/tools/obj2yaml/linker-options.yaml | ||
61 | I see why you want to use NUL, but I am not sure is this place really wrong? wiki says: 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? | |
61–64 | 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"? | |
llvm/test/tools/yaml2obj/linker-options.yaml | ||
7 | --string-dump looks fine here, thanks, I've forgot about it.
Yes, I wanted to use an independent approach to test the bytes emmitted. | |
llvm/tools/obj2yaml/elf2yaml.cpp | ||
626 | 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 |
Yep. We have a test: https://github.com/llvm-mirror/llvm/blob/master/test/tools/llvm-readobj/elf-linker-options.ll.
It uses llc and doesn't check any broken cases.
I think it worth to convert it to use YAML and improve.
llvm/lib/ObjectYAML/ELFYAML.cpp | ||
---|---|---|
1353–1354 | 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: "" | |
llvm/test/tools/obj2yaml/linker-options.yaml | ||
61–64 | 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. | |
llvm/tools/obj2yaml/elf2yaml.cpp | ||
626 | 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? |
- Addressed review comments.
llvm/lib/ObjectYAML/ELFYAML.cpp | ||
---|---|---|
1353–1354 | I did it since it is not a point I'd like to argue hard about. | |
llvm/test/tools/obj2yaml/linker-options.yaml | ||
61–64 |
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.
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... :) |
llvm/test/tools/obj2yaml/linker-options.yaml | ||
---|---|---|
61–64 |
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'. |
llvm/test/tools/obj2yaml/linker-options.yaml | ||
---|---|---|
61–64 | 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'. | |
llvm/test/tools/yaml2obj/linker-options.yaml | ||
105 | Perhaps "The we produce" -> "This produces" |
- Rebased, addressed review comments.
llvm/test/tools/obj2yaml/linker-options.yaml | ||
---|---|---|
61–64 | OK, I've updated the tests to use suggested samples.
I am not sure we should: we can implement this failure only if we add an additional code to check this, right? But while we can dump something reasonable and while the dump is human-readable and while it can be used by obj2yaml without changes, I believe we are on track. |
LGTM, with one remaining nit. Thanks!
llvm/test/tools/obj2yaml/linker-options.yaml | ||
---|---|---|
67 | contains an incomplete |
llvm/include/llvm/ObjectYAML/ELFYAML.h | ||
---|---|---|
150 | Use a trailing comma so that next time the diff can contain one fewer line. |
Use a trailing comma so that next time the diff can contain one fewer line.