This is an archive of the discontinued LLVM Phabricator instance.

Fix Driver/modules.cpp test to work when build directory name contains '.s'
ClosedPublic

Authored by tstellar on Aug 13 2019, 3:13 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

tstellar created this revision.Aug 13 2019, 3:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 3:13 PM
dyung added a comment.Aug 13 2019, 3:39 PM

It's probably a pathological case, but wouldn't this still fail if the build directory contained ".s" followed by a space in the name? Something like "build-foo.s bar"?

tstellar updated this revision to Diff 214960.Aug 13 2019, 4:04 PM

Another attempt to fix this that depends less on the build directory.

dyung added a comment.Aug 13 2019, 4:35 PM

Taking a step back and thinking about this, the compile command uses "-S", but explicitly specifies the output file to be "%t/module.pcm.o" (even though it is generating an assembly file instead of an object file), could we change the test to just check for "-o {{.*}}module.pcm.o "? That would work in all cases I think except for the extreme corner case where that string was part of the build directory name.

clang/test/Driver/modules.cpp
19 ↗(On Diff #214960)

I'm not sure what the regex module{{2*}} is supposed to match, or prevent from matching? Was this intentional?

It seems it would match "module", "module2", "module22", "module222", etc. When would the compiler ever generate anything other than the first? Unless you are trying to protect yourself against a build directory that contains module in the name, but I'm not sure how this helps that...

mgorny added a subscriber: mgorny.Sep 21 2019, 2:00 AM

Ping. This is a really silly issue, and one that could be fixed trivially, so it'd be really nice to actually fix it.

tstellar updated this revision to Diff 222205.Sep 27 2019, 10:51 AM

Don't check .s suffix.

tstellar marked an inline comment as done.Sep 27 2019, 10:53 AM
tstellar added inline comments.
clang/test/Driver/modules.cpp
19 ↗(On Diff #214960)

This CHECK line is used by two different RUN lines, the first writes to module.pcm.o and the second writes to module2.pcm.o.

dyung accepted this revision.Sep 27 2019, 2:20 PM

LGTM

This revision is now accepted and ready to land.Sep 27 2019, 2:20 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2019, 4:41 PM