GHC doesn't handle ./ prefixed paths correctly (#12674)
ClosedPublic

Authored by RolandSenn on Jul 26 2018, 10:27 AM.

Details

Summary

If a filename starts with a hypen, GHC keeps the prefixed "./" path.

Test Plan

make test TEST=T12674

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
RolandSenn created this revision.Jul 26 2018, 10:27 AM
bgamari edited the summary of this revision. (Show Details)Jul 26 2018, 4:16 PM
bgamari edited the test plan for this revision. (Show Details)
Phyx requested changes to this revision.Jul 27 2018, 6:18 PM

Hi @RolandSenn Thanks for the patch! Just some minor changes and it should be good to go.

ghc/Main.hs
225

why not just test the two patterns fully here? "./-" isPrefixOf` fp || ".\-" ...`. that should be easier to follow.

testsuite/tests/driver/T12674/-T12674.hs
7

Only GHC will run these so this define isn't needed.

testsuite/tests/driver/T12674/Makefile
7 ↗(On Diff #17457)

you don't need these RM commands, tests are run in their own individual folders, so you'll never have left overs from previous runs.

testsuite/tests/driver/T12674/all.T
3

don't think you need a Makefile and run_command for these. multi_compile_and_run with ['./-T12674.hs', [('./././-T12674c.c','')], ''] should work.

This revision now requires changes to proceed.Jul 27 2018, 6:18 PM
RolandSenn updated this revision to Diff 17491.Jul 28 2018, 2:18 PM
RolandSenn marked an inline comment as done.Jul 28 2018, 2:24 PM

Thanks for the code review and your comments. Please look at my replies and feel free to take your decisions.
Roland

ghc/Main.hs
225

On Windows, I fully agree with your remark!
On Linux the pathSeparator function returns /. Hence my version is the same as:

"./" isPrefixOf nfp || "./" isPrefixOf nfp

I intentionally don't test on backslashes on linux, because they don't have a special meaning.

Your suggestion changes the semantics of the do-we-have-a current-directory-at-the beginning-of-the-filename subtest.

Do you still want me to change?

testsuite/tests/driver/T12674/-T12674.hs
7

My test checks that GHC correctly handles file names prefixed with a hyphen in 4 situations:
1.) Haskell source file
2.) Calling the preprocessor.
3.) Calling the C-compiler.
4.) Calling the linker.

When calling an external program, we have to pass the file name also prefixed with the current directory (./).

To have a testcase for the preprocessor case I added the conditional language and the CPP pragma. The details of the #if logic are not important. Important is, that the preprocessor runs!

If I remove the #if and #endif, I have a redundant CPP pragma, which is bad.

Do you still want me to remove the conditional language?

testsuite/tests/driver/T12674/all.T
3

Good point!
I changed the first T12674 test according your suggestion and it runs without problems.
I got into troubles however, when I run the modified T12674w testcase.
Therefore I commented it out and restored the old version with the make file.

I think, here we have a problem in the testdriver:
In a new directory T12674Alt, I renamed the -T12674.hs and -T12674c.c files to names without the leading hyphens. (T12674.hs and T12674c.c) and also renamed the T12674.stdout file to T12674Alt.stdout .

Then the test runs successfully under Windows with the following all.T file: (Note the .\\ prefix in the extra_files section.)

test('T12674Alt', [extra_files(['.\\T12674.hs', 'T12674c.c']),
                unless(opsys('mingw32'), skip)],
                multi_compile_and_run, ['T12674.hs', [('T12674c.c', '')], '-v0'])

Then in the multi_compile_and_run line I add the .\\ prefix to T12674.hs.
The all.T file now reads:

test('T12674Alt', [extra_files(['.\\T12674.hs', 'T12674c.c']),
                unless(opsys('mingw32'), skip)],
                multi_compile_and_run, ['.\\T12674.hs', [('T12674c.c', '')], '-v0'])

Now the test failes with .T12674.hs not found. Note the dot (.) in front of the filename! Why does .\\T12674.hs work in the first line, but not in the third line?
Should I open a new Trac issue?

Phyx added inline comments.Jul 28 2018, 3:51 PM
ghc/Main.hs
225

Well now you're testing the same pattern twice on non-windows. checking the prefix 3x seems a bit much but I suppose it's fine since the prefixes will always be small.

I think I would have still prefered conditional logic here using

#if defined(mingw32_HOST_OS) or the values in the Platform module.

But I can live with this.

testsuite/tests/driver/T12674/-T12674.hs
7

No it's fine, just add a comment here saying why it's there.

testsuite/tests/driver/T12674/all.T
3

You likely just need to escape it twice .\\\\T12674.hs. Does that work?

RolandSenn updated this revision to Diff 17504.Jul 29 2018, 7:18 AM
RolandSenn marked 3 inline comments as done.

Many thanks for your kind help. I have now fixed all your issues. Please inform me, if you see other necessary changes.

ghc/Main.hs
225

I changed to conditional language.

testsuite/tests/driver/T12674/-T12674.hs
7

I added the comment.

testsuite/tests/driver/T12674/Makefile
7 ↗(On Diff #17457)

In mean time Makefile has gone. I will remember your advice for next time.

testsuite/tests/driver/T12674/all.T
3

Three backslashes also work. Why didn't I think to try this???

This revision is now accepted and ready to land.Jul 29 2018, 8:39 AM
This revision was automatically updated to reflect the committed changes.