cc65 / cc65

cc65 - a freeware C compiler for 6502 based systems
https://cc65.github.io
zlib License
2.31k stars 432 forks source link

Spaces in cl65 source filenames under Windows #788

Closed fadden closed 5 years ago

fadden commented 6 years ago
C:\src\cc65-2.17\bin\cl65.exe --target none ".\Name WithSpaces.S"
ca65: Don't know what to do with `WithSpaces.S'

Works fine under Linux.

The above was a simple test using the git shell, but I'm also launching it from SourceGen. Simply wrapping the filename works fine for Merlin 32 and 64tass, so I'm wondering if this is an issue with cl65 failing to quote filenames when passing them on to the other executables. Since it works under Linux I assume there's some difference in the shell command handler.

I found issue #540, but that seemed to be an installation directory issue.

mrdudz commented 6 years ago

we had this problem a while ago with VICE - the difference is that in *nix (and any other sane OS) its the shell that does the quoting, ie a string in quotes will be passed to the program in one argument. on windows however, that is NOT the case, the program (every program) must implement this all by itself (which the cc65 tools dont do apparently).

michaelcmartin commented 6 years ago

Windows provides three functions for doing this parsing:

Since the filesystem itself will be using wide characters, converting to a multibyte locale is probably unwise unless you're converting it to some normalized persistent UTF8 form or something.

Documents for CommandLineToArgvW and how it manages quotes and backslashes is on MSDN at https://docs.microsoft.com/en-us/windows/desktop/api/shellapi/nf-shellapi-commandlinetoargvw

groessler commented 5 years ago

@mrdudz: What you say doesn't seem to be the case. See

[2246064K] 0,0 c:\users\chris\tmp\ct>type ct.c
/* Testprogramm  für          */
/* Kommandozeile              */
/* 17-Nov-94, 01:07:13, chris */

#include <stdio.h>

void main(int argc,char **argv)
{
  int i;

  printf("Number of command line arguments: %d\n",argc);
  for (i=0;i<argc;i++) {
    printf("Commandline argument %d: \"%s\"\n",i,*(argv+i));
  }
  printf("\n");
}

[2247348K] 0,0 c:\users\chris\tmp\ct>cl -Fect.exe ct.c
Microsoft (R) 32-bit C/C++ Optimizing Compiler Version 13.10.3077 for 80x86
Copyright (C) Microsoft Corporation 1984-2002. All rights reserved.

ct.c
Microsoft (R) Incremental Linker Version 7.10.3077
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:ct.exe
ct.obj

[2254660K] 0,0 c:\users\chris\tmp\ct>ct parm1 "parm2 space" parm3
Number of command line arguments: 4
Commandline argument 0: "ct"
Commandline argument 1: "parm1"
Commandline argument 2: "parm2 space"
Commandline argument 3: "parm3"

[2278924K] 0,0 c:\users\chris\tmp\ct>

I also compiled it with a MingW version of 2012, with the same result.

My first guess would be that the cl65 does something wrong when it spawns the compiler. But since it seems to work on Linux (I haven't tested it), some investigations are needed. But it doesn't look like a deficiency in Windows.

mrdudz commented 5 years ago

as said, we had the same problem in vice. it also depends on the CRT that is linked (and mingw fixes it internally)

here is some of the monkey dance: https://sourceforge.net/p/vice-emu/code/HEAD/tree/tags/v3.2/vice/src/arch/win32/winmain.c

groessler commented 5 years ago

What compiler and CRT are you typically using?

mrdudz commented 5 years ago

this was with with visual studio compiled binaries iirc (i am not using windows usually, when i do its in msys2, and that doesnt have this problem i think)

groessler commented 5 years ago

i am not using windows usually

Me too :-)

I think I've got Visual Studio installed on my Windows VM as well. The "open" edition. I've just used the old MSVC7.1 compiler for my test above since that's what I'm used to. I can try again this command line test program with recent VS.

groessler commented 5 years ago

ct.c works the same with VS2015

Compyx commented 5 years ago

Please ignore, I got .s and .c mixed up.

groessler commented 5 years ago

For the record, I tested ct both with cmd.exe and tcc.exe, and it worked in both cases.

groessler commented 5 years ago

I cannot reproduce your problem, see (on XP, though):

commands

In your example you are running C:\src\cc65-2.17\bin\cl65.exe. Did you compile that locally? How did you compile it?

fadden commented 5 years ago

The binary I'm using was one I built, with Visual Studio Community 2017, using cc65.sln from the project (with a separate compile for the makefile-driven library stuff). I downloaded the pre-built Windows version from SourceForge just now ("cl65.exe V2.17 - Git 462d809") and confirmed that it does work correctly. So somehow the pre-built version is different from what I built out of cc65.sln.

FWIW, if I give it the name of a nonexistent file, the error messages show that cl65 sees the spaces, but ca65 doesn't get them:

> c:\src\cc65-2.17\bin\cl65.exe --target none ".\With Spaces.S"
ca65: Don't know what to do with `Spaces.S'
> c:\src\cc65-2.17\bin\cl65.exe --target none "foo bar"
cl65.exe: Don't know what to do with `foo bar'

(Incidentally, it would be great to add the Windows snapshot to the Releases page on github. I still have trust issues with SourceForge.)

groessler commented 5 years ago

I've never compiled cc65 by using the cc65.sln file. For obvious reasons, since I'm working Linux-based. Calling Oliver to the rescue :-)

oliverschmidt commented 5 years ago

If the current understanding is that the issue at hand is only present when building the cc65 binaries with Visual C++ (in contrast to MinGW) then I personally strongly suggest to just close the issue. From my POV the only purpose of the Visual Studio .sln file is to allow to debug cc65 using the superior Visual Studio debugger. And in that scenario the issue at hand can be very easily workarounded by putting cc65 in directory without spaces.

groessler commented 5 years ago

Oliver, I tend to disagree.

If the files are there, they should work. People are going to use it (see the issue at hand), and won't know that this is not the preferred way to build the tools. Probably it's just a stupid setting on one of the many confusing "project settings" panels in VS. When I had to work on Windows in my work, I always created Makefiles to build the stuff and never relied on a GUI build. I didn't trust it. But that's just me.

oliverschmidt commented 5 years ago

I'm not against fixing the issue but I'm not interested to personally invest time. And I'm against removing the Visual Studio solution because I want it for debugging purposes - as pointed out.

groessler commented 5 years ago

I see. But still, my argument "People are going to use it" is still valid. Could you move the .sln file somewhere out of the way (into a subdirectory, along with a README explaining its purpose)? Or find some other way to tell people not to use it?

oliverschmidt commented 5 years ago

Come on, it's not that the binaries created with the .sln file are broken. It's just that they don't properly support spaces in the path. Sorry, but to me this whole thing is pretty much an overreaction.

groessler commented 5 years ago

And, to add, I personally consider file or directory name with spaces, evil. But I have no control what other people are doing.

groessler commented 5 years ago

I don't consider them "broken" per se. But they have a deficiency in this regard. How long did it take in this thread to find out that the OP build with the .sln file? Takes time and energy from all of us when then next report appears...

oliverschmidt commented 5 years ago

https://cc65.github.io/getting-started.html says

"Download the current cc65 snapshot and unzip it to a path of your choice that doesn't contain spaces (let's presume 'c:\cc65' here). Now cc65 is fully functional without further steps."

So:

I don't want to have dispute for this, but I really see no need to do anything at all about the current state of things!

groessler commented 5 years ago

No user has ever been told that he's supposed to be able to build cc65 hisself in the first place.

C'mon, that's the whole point of "open source". If I hadn't been able to build cc65 locallly, I'd never had made any contribution to it.

The user has been told that cc65 is supposed to run from a directory without spaces.

That's different, and doesn't state that source file names also shouldn't contain spaces. I myself despise spaces in filenames myself, but I don't want to force my opinion onto anybody else.

I really see no need to do anything at all about the current state of things

What's the problem? A stupid bug which I guess could be fixed in less than an hour. And, after all, the .sln and .vcxproj files are your invention.

groessler commented 5 years ago

And, yes, of course. We could write in the docs that source file names mustn't contain spaces. That woul d be fine for me as well.

oliverschmidt commented 5 years ago

C'mon, that's the whole point of "open source".

You are of course very right. Create a fork and remove the Visual Studio files.

A stupid bug which I guess could be fixed in less than an hour.

As I said I'm not against a fix. If you can provide it I'm happy to have it.

And, after all, the .sln and .vcxproj files are your invention.

If I'm made personally responsible for every "issue" I may have introduced then it's the right time to stop maintaining this project. You see, no fork needed in the first place ;-)

We could write in the docs that source file names mustn't contain spaces.

You found out that this isn't the case with a properly built cc65. So I don't see why this should be part of the "normal" docs.

groessler commented 5 years ago

Let's step back a little bit. I didn't want to come over as aggressive.

You are of course very right. Create a fork and remove the Visual Studio files.

I obviously don't want to do that.

As I said I'm not against a fix. If you can provide it I'm happy to have it.

For me to make a fix would be a major effort. I don't have an up-to-date Windows development environment at hand, but I think you do. "major effort" in the time spent to get an environment setup and learn it.

If I'm made personally responsible for every "issue" I may have introduced then it's the right time to stop maintaining this project.

If I create an "issue" in the things I contribute, I typically feel responsible for it. And I experienced you to feel the same when it affects AppleII issues. Why is it different with these VS files?

You found out that this isn't the case with a properly built cc65.

What exactly consitutes an "properly built cc65"?

oliverschmidt commented 5 years ago

You found out that this isn't the case with a properly built cc65. So I don't see why this should be part of the "normal" docs.

The only place I'm aware of dealing with building cc65 on Windows is https://github.com/cc65/wiki/wiki/Microsoft-Windows. I just added "Note that cc65 built with Visual C++ appears to have issues with spaces in the names of source files." there. If that's not precise and/or detailed enough then everybody with write access to the Wiki is welcome to improve it. Everybody without write access to the Wiki is welcome to either request write access to it or add the improvement here and I'll add it to the Wiki.

oliverschmidt commented 5 years ago

If I create an "issue" in the things I contribute, I typically feel responsible for it. And I experienced you to feel the same when it affects AppleII issues. Why is it different with these VS files?

That has always been the difference between us two. I only fix the issues I'm interested in being fixed. Not the issues others tell me to fix. The latter I do for a living ;-)

groessler commented 5 years ago

That has always been the difference between us two. I only fix the issues I'm interested in being fixed.

Fair enough.

Not the issues others tell me to fix. The latter I do for a living ;-)

Yes, thats "work" :-)

I'm just trying to improve cc65 and have it work for "average user" without surprises.

oliverschmidt commented 5 years ago

I'm just trying to improve cc65 and have it work for "average user" without surprises.

I really appreciate that you're trying to provide the very type of cc65 support I'm not providing (and I made it very explicit right from the start that I'll never provide it). We both know which supportees I'm think of here ;-)

This thread is a great example: An issue on Windows while you personally aren't really that interested in Windows. Nevertheless you invest a significant amount of time. I admire that dedication!

But please don't try to make me do such stunts.

mrdudz commented 5 years ago

somehow this discussion makes me smile, reminds me of how compyx and myself have different ways of supporting VICE (i tend to agree with olivers approach :))

that said, it really should be a simple matter of tweaking the project settings in msvc. (the issue with vice was slightly different, as that was a "windows" not "console" application)

oliverschmidt commented 5 years ago
C:\src\cc65-2.17\bin\cl65.exe --target none ".\Name WithSpaces.S"
ca65: Don't know what to do with `WithSpaces.S'
  1. I can't reproduce this issue neither in CMD.EXE nor in the Git Shell. Neither with a .c file no with a .s file.

  2. The cl65 source already contains https://github.com/cc65/cc65/blob/master/src/cl65/main.c#L201-L217

  3. I'm using VS2017. So far I deliberately didn't update the solution files as I considered this a service towards other maybe using an older VS version. But from now on I'll just always update the solution files to the VS version I personally happen to use: https://github.com/cc65/cc65/commit/01857cd4defd74d1a084a6e85b746743d1271403

  4. I reverted my addition to the Wiki.

oliverschmidt commented 5 years ago

In case someone is looking for an area I personally consider more relevant:

https://github.com/cc65/cc65/issues/784 is a really ugly bug !!

E.g. I had to workaround it in https://github.com/cc65/ip65/blob/master/apps/ifttt.c#L84-L85 with

    *ptr = toascii(*ptr);
    ++ptr;

instead of

    *ptr++ = toascii(*ptr);
mrdudz commented 5 years ago

This is likely undefined behaviour, gcc warns about it:

 test.c:12:9: warning: operation on 'ptr' may be undefined [-Wsequence-point]
 *ptr++ = doit(*ptr);
  ~~~^~

http://c-faq.com/expr/evalorder1.html

784 is a different issue :)

oliverschmidt commented 5 years ago

Thanks for pointing out. This was totally unclear to me.

However, I could imagine VERY well that a fix for https://github.com/cc65/cc65/issues/784 will change the actual behavior of cc65 in the scenario above as a side effect.

Compyx commented 5 years ago

That would just push cc65 into the realm of "implementation defined behaviour", also not what you want, I think.

greg-king5 commented 5 years ago

Thanks for pointing it out. This was totally unclear to me.

However, I could imagine VERY well that a fix for #784 will change the actual behavior of cc65 in the scenario above as a side effect.

No, it won't. Compyx is correct; it's "implementation-defined" behavior. It has nothing to do with the optimizer.

When cc65 evaluates *ptr++ = toascii(*ptr);, it puts the original value of ptr on the stack, increments ptr, loads the (new) value of ptr, fetches from that address, calls toascii(), then stores the result to the (original) address that's on the stack.

oliverschmidt commented 5 years ago

@greg-king5: Thanks for the detailed explanation.