Split this change into 2.

I will try to split this change.

Addressed clang tests.

Address test failures.

Address clang-tidy warnings.

Hi, it looks like this change broke our (Fuchsia's) toolchain builder. We are seeing linker errors when we do stage2 builds:

In D96930#2570520, @MaskRay wrote:

an empty symbol table

It is not empty. It contains exactly one entry, a local symbol with index 0.

In D94461#2534861, @plotfi wrote:

Ping? Haven't seen any chatter in a while. Anything new here? I don't want to hold you folks up.

Added env TZ=GMT to force use GMT timezone. Not quite sure if it works on Windows. The default cmd.exe does not seem to support env VAR=VAL pattern to set environment variables.

I think I figured out the issue we saw. My local timezone is GMT-8 while @hans and @gargaroff are probably at GMT+1 . When touch is supplied with a time stamp 197001010000 it interpret ate it as local time, so the actual UTC time on my machine would be 197001010800 while on your machines it would be 196912312300, which is probably not valid based on how unix time work. I tried to modify my time zone to GMT+1 and I encountered the same issue:

In D92902#2527600, @hans wrote:

In any case, if it's working on your side, then there must be something wrong with my setup for some reason. I'll try to figure it out or just update the tests in our downstream repo. The Windows container does seem quite brittle after all. Sorry for bothering and thanks for checking anyway!

The test also fails on my machine, and I can reproduce the touch problem:

C:\src\tmp>touch --version
touch (GNU coreutils) 5.3.0
Written by Paul Rubin, Arnold Robbins, Jim Kingdon, David MacKenzie, and Randy Smith.

Copyright (C) 2005 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

C:\src\tmp>touch -m -t 197001010000 foo
touch: invalid date format `197001010000'

I also don't see it on any buildbots, but I'm not entirely sure why.

In any case, my machine is used to build the LLVM releases, and since this landed before the 12.0.0 branch point, it's now blocking the LLVM 12 release and we need to fix it somehow. Filed https://bugs.llvm.org/show_bug.cgi?id=48917 to track this.

In D92902#2525352, @gargaroff wrote:

Hi @haowei !
The tests are failing on Windows with GnuWin32 installed, because touch fails to parse the date string:

C:\GnuWin\bin\touch.exe: invalid date format `197001010000'

Could you take a look and fix this? I'm using the pre-merge-checks Windows container to test this.

In D93362#2523634, @MaskRay wrote:

@haowei Please re-test after rebasing. The SectionHeaderTable syntax has changed. I have fixed read-elf-dynsym.test

I plan to land this patch since the issue raised is a preexisting one and I prefer to address it in a separate change.

@MaskRay Do you have further comments on this change? Can I get an LGTM from you? Thanks.

In D93362#2500294, @grimar wrote:

In D93362#2499834, @haowei wrote:

For the case to test error message when sh_size % sh_entsize is not 0, I didn't find any good examples that I can manipulate the sh_entsize through yaml2obj. Do you have good suggestions to write unit tests for these cases?

Can you elaborate what the problem is? There are many examples in both yaml2obj and llvm-readelf test suites that are using the EntSize key to set the sh_entsize.

In D93362#2497437, @jhenderson wrote:

Is it important that the .dynsym is used directly when .hash/.gnu.hash is available? At the moment, your test doesn't really show whether it is used or whether you go straight to the dynamic tags for details, I believe - to do that, you'd need to have one given bogus values for the size, and show that this is not sued. Additionally, you have no testing for your error messages, nor do you have testing that shows that the .gnu.hash is used above the .hash section.

In D93362#2492168, @jhenderson wrote:

In D93362#2492101, @haowei wrote:

I rebased the code, it seems upset the Phabricator since it does not distinguish between rebase changes and actual changes.

What do you mean it seems upset? The review here looks fine to me.

If I compare Diff 2 and Diff 3 in Phabricator, the result looks weird as rebase changes were included.

fix some code style issues.

I rebased the code, it seems upset the Phabricator since it does not distinguish between rebase changes and actual changes.

An updated version was landed in f4c26d993bdc . This diff is no longer needed.

An updated version was landed in f4c26d993bdc . This diff is no longer needed.

An updated version was landed in f4c26d993bdc . This diff is no longer needed.

An updated version was landed in f4c26d993bdc . This diff is no longer needed.

An updated version was landed in f4c26d993bdc . This diff is no longer needed.

An updated version was landed in f4c26d993bdc . This diff is no longer needed.

friendly ping.

fix test on mac

In D92902#2474539, @thakis wrote:

Looks like this breaks tests on mac: http://45.33.8.238/macm1/897/step_10.txt

Please take a look, and revert for now if it takes a while to fix.

Change was reverted. Looks like touch on bsd behaves differently from the gnu one.

In D92902#2468951, @MaskRay wrote:

--preserve-dates in this utility is different from --preserve-dates in GNU/LLVM objcopy, so I am a bit unsure this is the right name. In llvm-tblgen such an option is called --write-if-changed

In D93362#2460124, @jhenderson wrote:

What others said - there are six cases we should care about:

1. Section headers are stripped and the ELF has no hash/gnu_hash dynamic tag - this can be treated as an error, I think since the ELF gABI requires a hash table.
2. Section headers are stripped, but there is a DT_HASH
3. Section headers are stripped, but there is a DT_GNU_HASH
4. Section headers present but there is no .hash or .gnu.hash section - this might be treated as an error, I think since the ELF gABI requires a hash table.
5. Section headers present with .hash section
6. Section headers present with .gnu.hash section

I think all of the tools should handle all of these situations (llvm-readelf, llvm-objdump, llvm-elfabi, ...) so it definitely seems like this should be part of the Object library.