This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho][test][nfc] Update stabs.s to use touch -d instead of -t
ClosedPublic

Authored by pzheng on Dec 13 2022, 4:28 PM.

Details

Summary

The test currently uses touch -t (e.g., `env TZ=UTC touch -t
"197001010000.16"`) to set file timestamps. However, this does not seem to set
the time zone correctly in a singularity container. While this is probably a
bug/limitation of the singularity container, but we can instead use `touch
-d` (e.g., touch -d "1970-01-01 00:00:16 UTC") to achieve the same result
without relying on a fix from singularity. Thoughts?

Diff Detail

Event Timeline

pzheng created this revision.Dec 13 2022, 4:28 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 13 2022, 4:28 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
pzheng requested review of this revision.Dec 13 2022, 4:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2022, 4:28 PM
int3 accepted this revision.Dec 13 2022, 4:56 PM

Assuming this works on all the buildbots, I don't see why not

This revision is now accepted and ready to land.Dec 13 2022, 4:56 PM

Thanks for reviewing, @int3!

haowei added a subscriber: haowei.Dec 13 2022, 9:55 PM

This change breaks lld/test/MachO/stabs.s on Fuchsia's Mac x64 Clang builder:

Failure message:

--
Exit Code: 1

Command Output (stderr):
--
touch: illegal option -- d
usage:
touch [-A [-][[hh]mm]SS] [-acfhm] [-r file] [-t [[CC]YY]MMDDhhmm[.SS]] file ...

--

Failed builder task: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-mac-x64/b8794839433510130609/overview

Looks like -d flag is not supported by the touch under Mac Big Sur, which is used by the builder above.
I tried the same command on my Ventura Mac and it still fails:

➜  ~ touch -d "1970-01-01 00:00:16 UTC" a
touch: out of range or illegal time specification: YYYY-MM-DDThh:mm:SS[.frac][tz]
➜  ~

Due to this inconsistency under different mac os versions, I think we should avoid use -d flag. Could you revert this change first while trying a different approach please?

man touch looks like -d wants simplified ISO 8601 form on macOS (and BSD: https://www.freebsd.org/cgi/man.cgi?query=touch&sektion=1) This works on my mac: touch -d "1970-01-01T00:00:16Z" So maybe that could work.

But as you say, that sounds like a bug in your container. Have you reported it to their upstream?

pzheng added a comment.EditedDec 14 2022, 9:59 AM

Thanks for acting upon this quickly, @thakis!

Sorry for the inconveniences caused by this change, @haowei. Do you mind trying the other form of the command (touch -d "1970-01-01T00:00:16Z") on your builder and see if that works? I am hoping that we can have a simple workaround for the container issue I encountered without breaking any other stuff of course.

Thanks for acting upon this quickly, @thakis!

Sorry for the inconveniences caused by this change, @haowei. Do you mind trying the other form of the command (touch -d "1970-01-01T00:00:16Z") on your builder and see if that works? I am hoping that we can have a simple workaround for the container issue I encountered without breaking any other stuff of course.

I don't have ssh access on builder bots, unfortunately. If you look at the error output, -d flag looks like is not supported at all on Big Sur. In that case, using -d will break LLVM build on all Big Sur or older Mac OS and it would be undesirable.

If this is the only test your container has trouble has. How about using LIT_FILTER_OUT env arg to bypass this test in your container?

Thanks for acting upon this quickly, @thakis!

Sorry for the inconveniences caused by this change, @haowei. Do you mind trying the other form of the command (touch -d "1970-01-01T00:00:16Z") on your builder and see if that works? I am hoping that we can have a simple workaround for the container issue I encountered without breaking any other stuff of course.

I don't have ssh access on builder bots, unfortunately. If you look at the error output, -d flag looks like is not supported at all on Big Sur. In that case, using -d will break LLVM build on all Big Sur or older Mac OS and it would be undesirable.

If this is the only test your container has trouble has. How about using LIT_FILTER_OUT env arg to bypass this test in your container?

I see. Thanks for helping, @haowei! Looks like I'll have to think about other alternatives. :)

I did more experiments with the command. Looks like replacing UTC with GMT worked correctly in the container. For example, env TZ=GMT touch -t "197001010000.16". This is functionally equivalent to env TZ=UTC touch -t "197001010000.16" and probably won't break anything. Any concern with this approach?

Wondering what is the error you see when uses UTC? While there are some differences, UTC and GMT basically means the same thing. So I am really surprised one works fine while the other will fail.

FYI, the command you supplied works fine on Mac OS Monterey and Ventura.

Thanks for checking, @haowei!

I noticed this test failure only when running the test in a singularity container (guest os: Ubuntu16 and host os: SLES12). To be clear, this test runs fine on the same host outside the container. The failure seems to be a result of the container failing to honor the UTC time zone when executing the touch command. For example, here is what it looks like when running touch inside the container.

Singularity> env TZ=UTC touch -t "197001010000.16" /tmp/test.o
Singularity> stat /tmp/test.o
  File: '/tmp/test.o'
  Size: 1256            Blocks: 8          IO Block: 4096   regular file
Device: fe03h/65027d    Inode: 180093152   Links: 1
Access: 1970-01-01 00:00:16.000000000 -0800
Modify: 1970-01-01 00:00:16.000000000 -0800
Change: 2022-12-14 17:28:25.501357159 -0800

As you can see, the modification time of the file was not set correctly. Looks like the container messed up the time zone? Instead of using UTC time zone, the container seemed to use the default time zone which is PST. Running the same command outside the container on the same host shows the correct result.

>env TZ=UTC touch -t "197001010000.16" /tmp/test.o
>stat /tmp/test.o
  File: '/tmp/test.o'
  Size: 1256            Blocks: 8          IO Block: 4096   regular file
Device: fe03h/65027d    Inode: 180093152   Links: 1
Access: 1969-12-31 16:00:16.000000000 -0800
Modify: 1969-12-31 16:00:16.000000000 -0800
Change: 2022-12-14 17:37:48.101538639 -0800

This looks like a bug of the singularity container since executing the same command on the same host outside the container produces correct results.

The workaround I found is to set the time zone to GMT instead of UTC. The container seems to honor TZ=GMT correctly.

Singularity> env TZ=GMT touch -t "197001010000.16" /tmp/test.o
Singularity> stat /tmp/test.o
  File: '/tmp/test.o'
  Size: 1256            Blocks: 8          IO Block: 4096   regular file
Device: fe03h/65027d    Inode: 180093152   Links: 1
Access: 1969-12-31 16:00:16.000000000 -0800
Modify: 1969-12-31 16:00:16.000000000 -0800
Change: 2022-12-14 17:46:03.521219660 -0800

I just proposed the new workaround in D140233.

This is clearly a container bug and the workaround doesn't make sense to me. TZ=UTC is used in 2 other tests as well. It seems that TZ=UTC is used at least as much as TC=GMT and likely more in the wild.
It seems that your container for whatever reason drops /usr/share/zoneinfo/**/UTC files?
GMT uses /usr/share/zoneinfo/**/GMT. I am not sure this workaround is acceptable.
You probably need to bare with test failures if your container does this.