This is an archive of the discontinued LLVM Phabricator instance.

[llvm-elfabi] Emit ELF header and string table section
ClosedPublic

Authored by haowei on May 9 2019, 3:54 PM.

Details

Summary

This change serves to create the initial framework for outputting ELF files from llvm-elfabi.

Sorry this is so large but this, perhaps short of the code for outputting the string table, is about as small of a setup as I can get and still have something testable. You can see the full picture here: https://reviews.llvm.org/D55864. Had I tried to get to exactly this point more directly and not by splitting it probably wouldn't be quite as large or generically written but if possible I'd like to keep some of the generality here since I'll likely add it back soon.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
haowei updated this revision to Diff 295370.Sep 30 2020, 12:10 PM
haowei marked 2 inline comments as done.

Resolve clang-tidy warnings.

haowei updated this revision to Diff 298255.Oct 14 2020, 3:52 PM

Rebase the change and fixes some format issues.

The purpose of this tool is still a bit unclear to me. Would you mind explaining a little? If you want to create a shared object writer, you can start from yaml2obj. Then there is natural question what generation features this tool can provide while yaml2obj cannot. If you want to write an interface shared object, sh_addr can be 0.

The purpose of this tool is still a bit unclear to me. Would you mind explaining a little? If you want to create a shared object writer, you can start from yaml2obj. Then there is natural question what generation features this tool can provide while yaml2obj cannot. If you want to write an interface shared object, sh_addr can be 0.

We've had this discussion multiple times in the past so I'm not sure how much of that discussion we want to replay here. The somewhat short version is that yaml2obj is a testing tool intended to be used only within LLVM whereas llvm-elfabi is intended as a production tool shipped with your toolchain. The scope of these tools is also different: llvm-elfabi can generate a machine readable ABI description (we use YAML but that's an implementation detail, we're also considering other formats) for a given binary and then generate a stub from that description; yaml2obj only handles the latter part, but at the same time it also handles much broader range of use cases that are needed for testing and the YAML format it accepts has a lot of features we never want to expose to users. While I agree that there's some overlap between these tools, their use case is sufficiently different to warrant different tools in our opinion. There might be an opportunity for deduplicating some of the implementation, but that's also the case for llvm-objcopy/llvm-strip, and it's something we would like to handle separately in the future.

MaskRay added a comment.EditedOct 23 2020, 11:09 AM

The purpose of this tool is still a bit unclear to me. Would you mind explaining a little? If you want to create a shared object writer, you can start from yaml2obj. Then there is natural question what generation features this tool can provide while yaml2obj cannot. If you want to write an interface shared object, sh_addr can be 0.

We've had this discussion multiple times in the past so I'm not sure how much of that discussion we want to replay here. The somewhat short version is that yaml2obj is a testing tool intended to be used only within LLVM whereas llvm-elfabi is intended as a production tool shipped with your toolchain. The scope of these tools is also different: llvm-elfabi can generate a machine readable ABI description (we use YAML but that's an implementation detail, we're also considering other formats) for a given binary and then generate a stub from that description; yaml2obj only handles the latter part, but at the same time it also handles much broader range of use cases that are needed for testing and the YAML format it accepts has a lot of features we never want to expose to users. While I agree that there's some overlap between these tools, their use case is sufficiently different to warrant different tools in our opinion. There might be an opportunity for deduplicating some of the implementation, but that's also the case for llvm-objcopy/llvm-strip, and it's something we would like to handle separately in the future.

This rationale is fine. However, this effort is a resurrection of a very old patch. Many tools have documentation under docs/CommandGuide and some file-level comments in the main source file while llvm-elfabi doesn't.
I think others may have similar concerns as I did. If you want to address such concerns and make the scope of the tool clear, I hope you can put the link the last discussion in this patch or have something in the documentation.

haowei updated this revision to Diff 301152.Oct 27 2020, 6:05 PM

merge public sections in ELFStubBuilder class

phosek added inline comments.Oct 28 2020, 12:29 AM
llvm/lib/InterfaceStub/ELFObjHandler.cpp
99–100

Consider this.

122

Nit: would it be possible to separate function bodies with empty lines? You could also consider using clang-format which should handle that.

123

Nit: don't use { and } when body has only a single statement.

130

Nit: No space.

148

Nit: don't use { and } only around single arm (also in this case they're unnecessary, see the comment above).

haowei updated this revision to Diff 301418.Oct 28 2020, 1:41 PM

address reviewer comments.

haowei marked 5 inline comments as done.Oct 28 2020, 1:41 PM
phosek accepted this revision.Nov 9 2020, 2:32 PM
phosek added a reviewer: grimar.

LGTM

This revision is now accepted and ready to land.Nov 9 2020, 2:32 PM
grimar added inline comments.Nov 9 2020, 11:54 PM
llvm/lib/InterfaceStub/ELFObjHandler.cpp
55

I think you can get rid of these usings.

Elf_Phdr and Elf_Shdr are used only once, so you can inline them:

ElfHeader.e_phentsize = sizeof(typename ELFT::Phdr);
ElfHeader.e_shentsize = sizeof(typename ELFT::Shdr);

The same for Elf_Ehdr + you can change memset to

memset(&ElfHeader, 0, sizeof(ElfHeader));
113

You can use TYPEDEF_ELF_TYPES(ELFT) instead of adding these usings.

124

Please don't use auto where type is not obvious.

129
std::vector<OutputSection<ELFT> *> Sections = { &ShStrTab, &StrTab };
133

Don't use auto.

136

It should be OK to add an empty string to ELF string tables. Do you really need if (!Sec->Name.empty()) condition?

142

addrs -> addresses

145

Don't use auto.

148

Why nobits sections are special?

Also I don't understand:

  1. There are just 2 sections currently: ShStrTab and StrTab. Why do you need the code for NoBits?
  2. Why can't you set Align = 1 from start for them instead of having Sec->Align ? Sec->Align : 1; logic?
169

You don't need these 2 lines, because you do memset(&ElfHeader, 0, sizeof(Elf_Ehdr)); in initELFHeader.

536

You should be able to use std::string toString(Error E):

Stream << toString(BufOrError.takeError());

Then you don't need the FileReadError variable and consumeError.

I think you should be able to do just something like:

if (!BufOrError)
  return createStringError(errc::invalid_argument,
    toString(BufOrError.takeError()) + " when trying to open `" +  FilePath + "` for writing");
537

You don't have a test for this I think.

546

It is more common naming I think.

595

this should be llvm_unreachable I think?

llvm/test/tools/llvm-elfabi/write-elf32be-ehdr.test
15 ↗(On Diff #301418)

Instead of using {{$}} everywhere, consider using FileCheck --match-full-lines.

29 ↗(On Diff #301418)

No new line.

llvm/tools/llvm-elfabi/llvm-elfabi.cpp
48

All above are unrelated formatting changes I think.

143

You don't need curly bracers around a single line:

if (SOName.getNumOccurrences() == 1)
   TargetStub->SoName = SOName;
haowei updated this revision to Diff 304391.Nov 10 2020, 8:56 PM
haowei marked 14 inline comments as done.
haowei updated this revision to Diff 304393.Nov 10 2020, 9:00 PM
haowei added inline comments.Nov 10 2020, 9:02 PM
llvm/lib/InterfaceStub/ELFObjHandler.cpp
113

I think this is only place in this change to use so many "using" statements. It is not very cost efficient to define a marco for this case.

148

Most part of this change was authored by my previous co-workers. I don't know the specific reason why NoBits are handled here, but I could guess the code for NoBits are reserved in case the stub SO generated from elfabi would need any sections that has NoBits attribute. I agree it is not useful now as the following change do not use NoBits sections as well.

For align, the sh_addralign of .shstrtab and .dynstr was set to to 0 in this change. If I recall correctly both 0 and 1 are acceptable values for sh_addralign if the section does not have alignment constraints. Since alignTo function require "Align" to be non-zero, I guess that is why Sec->Align ? Sec->Align : 1 exists. I have change the sh_addralign to 1 for the string tables to avoid it.

537

Emmm, I tried locally on a linux machine and it seems to be difficult to put it in test case.
I have to create a file with same filename, change ownership and the permission so elfabi cannot modify it. And run llvm-elfabi to write output to this file. Is there any good way to trigger error from FileOutputBuffer?

llvm/tools/llvm-elfabi/llvm-elfabi.cpp
48

Yes. I just run clang-format on entire file. Can I keep them?

Harbormaster completed remote builds in B78402: Diff 304393.
grimar added inline comments.Nov 10 2020, 11:44 PM
llvm/lib/InterfaceStub/ELFObjHandler.cpp
72

I'd use typename ELFT::Ehdr for consistency with lines below.

113

You are right. I thought we have it defined globally already (like LLVM_ELF_IMPORT_TYPES_ELFT), but it turns out it is only defined in llvm-readobj\ELFDumper.cpp currently.

537

Perhaps I am missing something (I am not sure what the problem is).
We have a few tests in LLVM (e.g see LLD tests, like lld\test\COFF\thinlto-emit-imports.ll) that
do something very close to what you describe I think: they create a file, change permission and run tool on it:

; RUN: rm -f %t3.obj.imports
; RUN: touch %t3.obj.imports
; RUN: chmod 400 %t3.obj.imports
; RUN: not lld-link -entry:main -thinlto-index-only \
; RUN:     -thinlto-emit-imports-files %t1.obj %t2.obj %t3.obj \
; RUN:     -out:%t4.exe 2>&1 | FileCheck %s --check-prefix=ERR
; ERR: cannot open {{.*}}3.obj.imports: {{P|p}}ermission denied

Can you do something like that?

llvm/test/tools/llvm-elfabi/write-elf64be-ehdr.test
2 ↗(On Diff #304393)

Use --match-full-lines too?

llvm/test/tools/llvm-elfabi/write-elf64le-ehdr.test
2 ↗(On Diff #304393)

Use --match-full-lines too?

llvm/tools/llvm-elfabi/llvm-elfabi.cpp
48

It is better to avoid unnecessarily changes in patches. These changes are unrealated and the right way
to do this would be to format the entry file first and commit as NFC, that should not need a review in this case.

haowei updated this revision to Diff 304652.Nov 11 2020, 2:14 PM
haowei marked 4 inline comments as done.
haowei added inline comments.Nov 11 2020, 2:16 PM
llvm/lib/InterfaceStub/ELFObjHandler.cpp
537

I added "fail-file-write.test".
I thought it is discouraged to use rm, chmod commands since they are not cross-platforms.

Do I have to add anything to prevent this test from running on Windows?

grimar accepted this revision.Nov 12 2020, 12:06 AM

LGTM with a nit.

llvm/lib/InterfaceStub/ELFObjHandler.cpp
537

Do I have to add anything to prevent this test from running on Windows?

I believe - no. touch and rm are often used in tests. chmod is a bit more rare, but
all of them are parts of GnuWin32 and works fine under windows.

llvm/test/tools/llvm-elfabi/fail-file-write.test
8

The last line has an excessive full stop at the end. I am not sure you need this line though.

jhenderson requested changes to this revision.Nov 12 2020, 1:52 AM

Nothing grievously wrong here, but lots of small things that need improving. Requesting changes mostly to clear the "Accepted" state.

llvm/lib/InterfaceStub/ELFObjHandler.cpp
52

It's been a while since I looked at this code, but I wonder whether this is essentially a duplication of code elsewhere in LLVM. Certainly I'd expect tools like LLD, llvm-objcopy and llvm-mc to have functionality that emits an ELF Header of some description. Do we really need to repeat it here?

Also, I wonder if it would be a little more flexible/traditional to return a locally constructed ElfHeader rather than take it as an input?

70–71

Dead code? You use memset to zero earlier. Same goes for the EI_ABIVERSION setting earlier.

118

I wonder if it would be less confusing if StrTab were to be called Dynstr? .strtab is traditionally the name of the static symbol string table.

136–149

If I follow this code correctly, this whole loop could be dramatically simplified, as suggested.

This appears to be setting the initial address to the end of the ELF header. It's certainly not the case that the ELF header has to always be assigned memory - if it doesn't appear in a program header in a linker script, it won't be for example, so the initial address would start from zero. I note also that the program doesn't appear to be emitting program headers at all, so the use of addresses seems a little confusing to me anyway.

148

I've forgotten any memory I have of the long-term goal with this tool, so this might be irrelevant, but is it possible that a user in the future will set an alignment of 0 explicitly and therefore this will fail?

150
153

I suggest using a generic term rather than the variable name to avoid rotting comments should variables get renamed.

165

I wonder if it would be easier for this function (with the help of its child functions) to update the offset as it goes along, rather than functions needing to precalculate where the thing should be written:

void write(uint8_t *Data) const {
  write(Data, ElfHeader);
  Data += ShStrTab.Shdr.sh_offset;
  ShStrTab.Content.write(Data);
  Data += StrTab.Shdr.sh_offset;
  StrTab.Content.write(Data);
  Data = ElfHeader.e_shoff;
  writeShdr(Data, ShStrTab);
  writeShdr(Data, StrTab);
}

template <class T> static void write(uint8_t *&Data, const T &Value) {
  *reinterpret_cast<T *>(Data) = Value;
  Data += sizeof(Value);
}

void writeShdr(uint8_t *Data, const OutputSection<ELFT> &Sec) const {
  write(Data, Sec.Shdr);
}

At that point, some of the helper methods might become completely unnecessary.

167

Here and below, would a loop over a list of Sections make more sense going forwards, like you already do above? I don't know if there are expected to be more sections added in the future, but if there are, such a loop would be more robust than having to remember to add everything in multiple places.

170

It might not be important, but it seems more natural to have the SHF_ALLOC seection headers before the non SHF_ALLOC stuff in the section header table (same goes for the sections themselves).

179

This line makes me very nervous that this code will fail miserably where the host endianness doesn't match the target endianness with non-single byte T types.

190
384

I'm concerned that this has been changed to llvm_unreachable. Is this function only able to be called with ELF binaries? What happens if a user passes in e.g. an archive or COFF file? createBinary can return more than just ELF types, which would lead to an assertion here. This case will want testing too. Also, it seem unrelated to this patch?

581

The style guide says to use lower-case strings where possible for error messages.

I think this case SHOULD be an llvm_unreachable? It's not possible for a user to hit this code as far as I can tell (alternatively, write a test for it...)

llvm/test/tools/llvm-elfabi/binary-write-sheaders.test
1 ↗(On Diff #304652)

Please add a comment to the top of this test stating what this test is testing. Same goes for all tests.

This test is only testing one of the 4 ELF formats (64-bit LE). You should probably have the other three too?

7 ↗(On Diff #304652)

Nit: remove extra blank line.

llvm/test/tools/llvm-elfabi/fail-file-write.test
16

Nit: delete extra blank line.

17

Please check the full error here, rather than a partial one. Additionally, I would be suspicious that the error message will be different depending on whether the host OS is Linux or Windows. Please could you check? If you don't have access to both systems, let me know, and I can test the one you can't.

llvm/test/tools/llvm-elfabi/invalid-bin-target.test
1 ↗(On Diff #304652)

I'd rename this test to invalid-output-target or even output-target-invalid to match the option name.

llvm/test/tools/llvm-elfabi/missing-bin-target.test
1 ↗(On Diff #304652)

It would probably be easier to merge this and the above test together, so that you have all tests for --output-target in one place.

10 ↗(On Diff #304652)

This probably wants an additional bit of message. Something like "error: No binary..."

llvm/test/tools/llvm-elfabi/write-elf32be-ehdr.test
1 ↗(On Diff #304652)

It's not clear to me why this test is separate to the binary-write-sheaders test? Can they be merged?

llvm/test/tools/llvm-elfabi/write-elf32le-ehdr.test
1 ↗(On Diff #304652)

If it doesn't have it already, I'd recommend adding a --docnum option to llvm-elfabi, like yaml2obj, so that you can have multiple test variants within the same file (in a separate prerequisite patch). In this case, I think it would make a lot of sense for it to be in the same test as the big endian test. Same with the 64-bit variants.

llvm/tools/llvm-elfabi/llvm-elfabi.cpp
48

As llvm-elfabi is a new tool, it would be better to do the reformatting early as @grimar suggests, rather than let things linger. That will make everybody's lives easier as you'll just be able to reformat the whole file when adding new code (and only the new bits will change).

140–144

This seems unrelated to your other changes in this patch? I certainly don't see any relevant test changes.

165

I can't remember exactly the invocation (and am too lazy to check it right now), but WithColor has a default error handler function that can be passed an LLVM error I believe, so use that instead, I recommend. I'd probably also wrap it all in a local function that does the exit(1) too to reduce duplication.

Also, LLVM style guide says errors have no full stops and no leading capitalization.

jhenderson added inline comments.Nov 12 2020, 1:52 AM
llvm/lib/InterfaceStub/ELFObjHandler.cpp
595

I think the wrong place was changed?

llvm/tools/llvm-elfabi/llvm-elfabi.cpp
171

Same comment here re. default error handler function. You might want to change the TBEWriteError and other error bits similarly, in a separate patch.

This revision now requires changes to proceed.Nov 12 2020, 1:52 AM
jhenderson added inline comments.Nov 12 2020, 1:53 AM
llvm/test/tools/llvm-elfabi/fail-file-write.test
17

Looking at the pre-merge checks this test is failing on Windows as-is.

grimar added inline comments.Nov 12 2020, 2:12 AM
llvm/lib/InterfaceStub/ELFObjHandler.cpp
179

It is only used for writing Elf_Shdr, Elf_Ehdr etc types, what I think should work fine on any hosts.
Is your concern that somebody might try to use write for regular types?

384

I believe this code is unreachable because readELFFile is only called for ELFs:

// First try to read as a binary (fails fast if not binary).
if (InputFileFormat.getNumOccurrences() == 0 ||
    InputFileFormat == FileFormat::ELF) {
  Expected<std::unique_ptr<ELFStub>> StubFromELF =
      readELFFile(FileReadBuffer->getMemBufferRef());
jhenderson added inline comments.Nov 12 2020, 2:28 AM
llvm/lib/InterfaceStub/ELFObjHandler.cpp
179

We're probably talking past each other, or I might be forgetting something, but the fields within Elf_Shdr, Elf_Ehdr etc will be in little/big endian order right depending on the host, right? This code here is essentially just doing a byte-wise copy, by my understanding.

384

InputFileFormat.getNumOccurrences() == 0 - doesn't that mean an unspecified input format will be attempted to be read as ELF, but there's no guarantee it actually is ELF...?

grimar added inline comments.Nov 12 2020, 2:41 AM
llvm/lib/InterfaceStub/ELFObjHandler.cpp
179

My understanding is the following:

E.g. Elf_Shdr consists of Elf_Word`/Elf_Addr/Elf_Off fields,
which are of support::detail::packed_endian_specific_integral type:

using Elf_Shdr_Base<ELFT>::sh_size;
  
template <endianness TargetEndianness>
struct Elf_Shdr_Base<ELFType<TargetEndianness, false>> {
  LLVM_ELF_IMPORT_TYPES(TargetEndianness, false)
  Elf_Word sh_name;      // Section name (index into string table)
  Elf_Word sh_type;      // Section type (SHT_*)
  Elf_Word sh_flags;     // Section flags (SHF_*)
  Elf_Addr sh_addr;      // Address where section is to be loaded
  Elf_Off sh_offset;     // File offset of section data, in bytes
  Elf_Word sh_size;      // Size of section, in bytes
  Elf_Word sh_link;      // Section type-specific header table index link
  Elf_Word sh_info;      // Section type-specific extra information
  Elf_Word sh_addralign; // Section address alignment
  Elf_Word sh_entsize;   // Size of records contained within the section
};

#define LLVM_ELF_IMPORT_TYPES_ELFT(ELFT)                                       \
  using Elf_Addr = typename ELFT::Addr;                                        \
  using Elf_Off = typename ELFT::Off;                                          \
  using Elf_Half = typename ELFT::Half;                                        \
  using Elf_Word = typename ELFT::Word;                                        \
  using Elf_Sword = typename ELFT::Sword;                                      \
  using Elf_Xword = typename ELFT::Xword;                                      \
  using Elf_Sxword = typename ELFT::Sxword;

  using Half = packed<uint16_t>;
  using Word = packed<uint32_t>;
  using Sword = packed<int32_t>;
  using Xword = packed<uint64_t>;
  using Sxword = packed<int64_t>;
  using Addr = packed<uint>;
  using Off = packed<uint>;
 
  template <typename Ty>
  using packed = support::detail::packed_endian_specific_integral<Ty, E, 1>;

This type respects endianess on assign:

struct packed_endian_specific_integral {
....
  void operator=(value_type newValue) {
    endian::write<value_type, endian, alignment>(
      (void*)Value.buffer, newValue);
  }

/// Write a value to memory with a particular endianness.
template <typename value_type, std::size_t alignment>
inline void write(void *memory, value_type value, endianness endian) {
  value = byte_swap<value_type>(value, endian);
  memcpy(LLVM_ASSUME_ALIGNED(
             memory, (detail::PickAlignment<value_type, alignment>::value)),
         &value, sizeof(value_type));
}

template <typename value_type>
inline value_type byte_swap(value_type value, endianness endian) {
  if ((endian != native) && (endian != system_endianness()))
    sys::swapByteOrder(value);
  return value;
}
384

Oh. I missed that there is || InputFileFormat == FileFormat::ELF, not && InputFileFormat == FileFormat::ELF.
This place looks a bit wierd to me..

haowei updated this revision to Diff 305052.Nov 13 2020, 12:38 AM
haowei marked 24 inline comments as done.
haowei updated this revision to Diff 305053.Nov 13 2020, 12:49 AM
haowei marked an inline comment as done.
haowei added inline comments.
llvm/lib/InterfaceStub/ELFObjHandler.cpp
52

I checked writeEhdr() in obj-copy but it is not flexible enough to be used here. I agree it would be great to have a common function to write Ehdr into memory so it can be reused by these tools. But I feel it is out of scope of this change. I can draft a change to address this problem if you feel it is necessary to do so.

118

StrTab is renamed to DynStr.

148

The section alignment is set in this builder function so I guess the case you discussed would not happen. I have no preference in using 0 or 1 here. Using 1 seems to make code simpler but less robust.

165

I prefer not to do that. Using write function to determine the offset would look very messy on D89432.

167

Using loop would make more sense but it seems to be infeasible in this case. The "Section.Content.write" function is responsible for writing data to the MemoryBuffer but this function is not a virtual function derived by different classes (there are SymTab and Dynamic sections builder in D89432) so it would require some data structure changes to make it happen. Or if there is anything like "instanceof" that I can check and cast the types safely at run time, that would also work.

As shown in D89432, we only need to write 4 sections in a stub elf file. So it is not very cost efficient to invent some new classes just to make the loop happen. If you have better solutions please let me know.

170

I changed the order in the new diff

179

Test does not fail when writing elfheader in be or le. If you have better solution to make it more robust, please let me know.

581

I changed it to llvm_unreachable.

595

I changed the wrong place, sorry about that.

llvm/test/tools/llvm-elfabi/fail-file-write.test
17

I have a windows workstation but it does not have development environment set up for llvm. I will see if I can set it up tomorrow. For now, let me test it on bots first.

llvm/test/tools/llvm-elfabi/missing-bin-target.test
1 ↗(On Diff #304652)

merged in latest diff.

llvm/test/tools/llvm-elfabi/write-elf32be-ehdr.test
1 ↗(On Diff #304652)

merged in latest diff.

llvm/test/tools/llvm-elfabi/write-elf32le-ehdr.test
1 ↗(On Diff #304652)

We intended to merge elfabi tool into elf-ifs tool once the we feel ELFObjHandler is stable. So I prefer not to add additional flag(feature) to llvm-elfabi at this point.

llvm/tools/llvm-elfabi/llvm-elfabi.cpp
48
140–144

writeBinaryStub also use SoName in TargetStub variable. This change moves this logic outside of the EmitTBE branch so the assignment at L140 can be written once but used by both L147 and L159.

165

I added a fatalError(Error err) function.

jhenderson added inline comments.Nov 16 2020, 2:07 AM
llvm/lib/InterfaceStub/ELFObjHandler.cpp
153

This comment still has most of the typos/grammar problems I fixed. Please review my suggested edit and fix them.

167

FWIW, LLVM has a dyn_cast still mechanism that allows some degree of runtime type checking, but it only works within a class heirarchy (see any class that has the classof function, and how that works).

It looks like it would be straightforward to unify the section types somehow, with a virtual write function, but without actually trying it, I'm not certain it would be.

179

It's fine. I forgot that the Elf_* types had endianness magic built into them.

567–579
llvm/test/tools/llvm-elfabi/missing-or-invalid-output-target.test
1 ↗(On Diff #305053)

Add descriptive comment at top of test. This should be in place for all tests.

I'd also consider renaming it output-target-error.test, which is a little more concise.

14 ↗(On Diff #305053)

Nit: no new line at EOF.

llvm/test/tools/llvm-elfabi/write-elf32be-stub.test
1 ↗(On Diff #305053)

Use '##' for comments, to make them stand out from other lit and FileCheck commands. This applies to all new comments you've added in this and other tests.

4–7 ↗(On Diff #305053)

These can all be combined into a single llvm-readobj execution, without the need for separate prefixes.

llvm/test/tools/llvm-elfabi/write-elf32le-stub.test
1 ↗(On Diff #305053)

It might make more sense to merge the ELF32/ELF64 and LE/BE tests all into a single test. Your test would look something like:

# RUN: llvm-elfabi %s --output-target=elf32-little %t32-little
# RUN: llvm-readobj -h -S --string-dump .shstrtab --string-dump .dynstr %t32-little | FileCheck %s -DCLASS="32-bit (0x1)" <other defines here for the other bits that are slightly different>
# RUN: llvm-elfabi %s --output-target=elf32-big %t32-b ig
# RUN: llvm-readobj -h -S --string-dump .shstrtab --string-dump .dynstr %t32-big | FileCheck %s -DCLASS="32-bit (0x1)" <other defines here for the other bits that are slightly different>
<same for 64-bit variants>
haowei updated this revision to Diff 305663.Nov 16 2020, 11:19 PM
haowei marked 3 inline comments as done.
haowei updated this revision to Diff 305673.Nov 17 2020, 12:19 AM
haowei marked 2 inline comments as done.
haowei marked 3 inline comments as done.Nov 17 2020, 12:22 AM
haowei added inline comments.
llvm/test/tools/llvm-elfabi/write-elf32le-stub.test
1 ↗(On Diff #305053)

I combined 4 test files into 1.

jhenderson added inline comments.Nov 17 2020, 12:59 AM
llvm/test/tools/llvm-elfabi/fail-file-write.test
17

Did you get anywhere with resolving this? The test is still failing on the pre-merge bot, as noted near the top of this page.

llvm/test/tools/llvm-elfabi/output-target-error.test
2
haowei updated this revision to Diff 305965.Nov 17 2020, 8:35 PM
haowei marked an inline comment as done.
haowei added inline comments.Nov 17 2020, 8:39 PM
llvm/test/tools/llvm-elfabi/fail-file-write.test
17

I finally get my windows workstation to build llvm successfully. So the reason this test failed on Windows is that, the chmod seems to be a no-op on windows. It didn't change the permission of the %t.TestDir at all. The failure at not llvm-elfabi... did not happen so the test failed.

I don't have a good workaround for this so I disabled the test under Windows for now. Do you have any good suggestion for testing this failure under Windows?

grimar added inline comments.Nov 18 2020, 12:00 AM
llvm/test/tools/llvm-elfabi/fail-file-write.test
17

I guess you should be able to change the permission for the file, if not for directory.

Our LLD test (ELF\lto\thinlto-cant-write-index.ll) has the following lines:

; Ensure lld generates error if unable to write to index files
; RUN: rm -f %t2.o.thinlto.bc
; RUN: touch %t2.o.thinlto.bc
; RUN: chmod u-w %t2.o.thinlto.bc
; RUN: not ld.lld --plugin-opt=thinlto-index-only -shared %t1.o %t2.o -o /dev/null 2>&1 | FileCheck %s
; RUN: chmod u+w %t2.o.thinlto.bc
; CHECK: cannot open {{.*}}2.o.thinlto.bc: {{P|p}}ermission denied

This test is not disabled for windows amd I believe that the last line has {{P|p}} because the message
produced is different on windows/*nix.

There are other LLD tests that are using chmod on files and are not disabled on windows.

haowei added inline comments.Nov 18 2020, 12:08 AM
llvm/test/tools/llvm-elfabi/fail-file-write.test
17

If I only change the file permission, it won't work under linux. Apparently the file write function used in elfabi will remove the file first and then recreate it. If the directory contains the file has wx permission, the file can be removed regardless of the file's own permission bits. That is why in this test, I changed the permission of the directory, not the file.

grimar added inline comments.Nov 18 2020, 12:28 AM
llvm/test/tools/llvm-elfabi/fail-file-write.test
17

I see. Perhaps it is OK to keep it unsupported at least for now.
(may be someone else have ideas, I don't have any currently).

I am not sure how your way of doing it works well though.
Usually in tests the "system-windows" is commonly used:

UNSUPPORTED: system-windows

I also observe "windows-gnu", "windows-msvc" and just "windows" versions but,
don't see "-windows-" anywhere.

jhenderson added inline comments.Nov 18 2020, 12:37 AM
llvm/test/tools/llvm-elfabi/fail-file-write.test
17

If you can get the test to work in one case for Linux and another case for Windows, how about you split this test into fail-file-write.test (for non-Windows) and fail-file-write-windows.test (for Windows) with the appropriate setup and REQUIERS/UNSUPPORTED checks according to the test?

haowei updated this revision to Diff 306244.Nov 18 2020, 3:49 PM
haowei updated this revision to Diff 306258.Nov 18 2020, 4:29 PM
haowei added inline comments.Nov 18 2020, 4:34 PM
llvm/test/tools/llvm-elfabi/fail-file-write.test
17

I added a windows test. However, it does not trigger the error from L530 in ELFObjHandler.cpp . Instead, it triggers the error from L539 in the same file. Maybe FileOutputBuffer::create is implemented differently on 2 platforms.

MaskRay added inline comments.Nov 18 2020, 4:48 PM
llvm/tools/llvm-elfabi/llvm-elfabi.cpp
125

Use static for these functions

https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

The coding standard also says non-exported class declarations should be written in anonymous namespaces

haowei updated this revision to Diff 306292.Nov 18 2020, 8:31 PM
haowei added inline comments.
llvm/tools/llvm-elfabi/llvm-elfabi.cpp
125

I added static.

This is not a class declaration I guess I don't need to put it into anonymous namespace.

jhenderson added inline comments.Nov 19 2020, 12:45 AM
llvm/test/tools/llvm-elfabi/fail-file-write.test
17

Windows file permissions are tricky at best. The important thing for these tests is that the code in llvm-elfabi that handles errors from FileOutputBuffer::create is correct. It doesn't really matter what the error actually is here.

llvm/test/tools/llvm-elfabi/output-target-error.test
2

Ping this?

llvm/tools/llvm-elfabi/llvm-elfabi.cpp
125

This is not a class declaration I guess I don't need to put it into anonymous namespace.

@MaskRay might be referring to the new classes in ElfObjHandler.cpp like OutputSection etc

haowei updated this revision to Diff 306527.Nov 19 2020, 1:48 PM
haowei marked 2 inline comments as done.
haowei added inline comments.Nov 19 2020, 1:55 PM
llvm/test/tools/llvm-elfabi/output-target-error.test
2

Sorry I missed that. Phabricator gray out the comments made me though I already fixed it.

llvm/tools/llvm-elfabi/llvm-elfabi.cpp
125

I put those classes into anonymous namespace. Does it look good?

MaskRay added inline comments.Nov 19 2020, 2:04 PM
llvm/lib/InterfaceStub/ELFObjHandler.cpp
194

The coding standard uses // end anonymous namespace (no period)
Some code uses // namespace because sometimes clang-format inserts that.
Please don't invent a new style.

llvm/tools/llvm-elfabi/llvm-elfabi.cpp
125

Please make all new functions static if not used outside the file.

haowei updated this revision to Diff 306536.Nov 19 2020, 2:25 PM
haowei marked an inline comment as done.
haowei marked an inline comment as done.Nov 19 2020, 2:27 PM
haowei added inline comments.
llvm/tools/llvm-elfabi/llvm-elfabi.cpp
125

All new functions that are not used outside of the translation unit was added with static modifier, please let me know if you found anything I missed.

jhenderson accepted this revision.Nov 20 2020, 12:14 AM

Looks good from my point of view, but please wait for @MaskRay to confirm he is happy.

This revision is now accepted and ready to land.Nov 20 2020, 12:14 AM
MaskRay accepted this revision.Nov 20 2020, 12:22 AM
This revision was landed with ongoing or failed builds.Nov 23 2020, 11:34 AM
This revision was automatically updated to reflect the committed changes.
haowei marked an inline comment as done.