Menu

#482 Fixing #3616

None
closed-fixed
None
5
2024-12-11
2024-10-25
No

The obvious way to fix [bugs:#3616] is to remove code that worked around the command.com-specific issues, which shouldn't be relevant anymore as the SDCC Windows binaries already depend on Windows XP or higher version, and the code for command.com would only be relevant if they'd run on Windows 95, 98 or ME.

Attached is a patch that removes the conversion to 8+3 short form of the program's full path and filename on Windows (which was made because of that old command.com convention), and instead unconditionally puts the double quotes both around the whole command line, and inside around the program with the path, which would work at least since Windows 2000. This is needed as the program with the path can contain spaces in the path or even the program name, and both mentioned quoting steps are needed for the proper functioning of invocations.

1 Attachments

Related

Bugs: #3616

Discussion

  • Janko Stamenović

    A small optimization, reverting to the fixed constant for the initial dbuf is good enough and generates less code (attached).

     
    • Philipp Klaus Krause

      Apart from some minor formatting issues, the patch and explanation make sense to me. But I don't know much about Windows. @epetrich, AFAIK you know a bit more about Windows than I do; what do you think?

      P.S.: I also like that uses of PATH_MAX are eliminated by the patch. PATH_MAX is not available on all OSes, and we currently just #define it to 2048 for those, which doesn't seem very safe.

       

      Last edit: Philipp Klaus Krause 2024-10-26
      • Janko Stamenović

        Regarding the PATH_MAX, the initial size for dbuf_initis just a guess, and ideally it should be "big enough to minimize later chance of reallocation", but doesn't have to be exact, as the dbuf_appendwhen needed calls realloc that's why I eventually kept the constant for dbuf_init (the second version in the comment). But PATH_MAX is not allocated on the stack in compose_command_line in my patch anymore, I agree that that's better.

         
      • Erik Petrich

        Erik Petrich - 2024-10-27

        I don't really have any deep insight into Windows specific development; it has been over 20 years since I've built SDCC under Windows and almost that long since I've used it to compile anything under a Windows environment. From time-to-time I have fixed some Windows specific problems enough to get the daily snapshots unbroken, mainly because no one else seemed willing.

        That being said, I have no objection to dropping work arounds needed for earlier versions of Windows and focusing support on Windows XP and later if that would improve the user experience with modern Windows.

         
        • Janko Stamenović

          Now I've checked also the SDCC's dependency on GetModuleHandleExW. This API doesn't exist on any Windows earlier than XP, and the SDDC's dependency comes from libstdc++-6.dll, and not from SDCC code directly calling it. I don't know since when that exists, but whenever that happened, is seems there were "no complaints" about SDCC non working on the earlier-than-XP Windows versions.
          Windows XP is generally available since August 2001.

           
    • Janko Stamenović

      A short additional explanation:

      The 8.3 name form is an original limitation of FAT file systems, a decision allowing them to be usable even on 8-bit computers of that time. Later, Microsoft introduced a solution allowing the older programs, which expected 8.3 name, to continue to work, while also allowing the newer programs to use longer names: Each time a file name had to be created, two different entries were created for the same file, both unique in the directory, and both had to be consistently maintained in the file system. That compatibility was maintained until recently, and also on other Microsoft's file systems, including NTFS (!)

      C:\WINDOWS\system32>fsutil fsinfo ntfsinfo c: | find "NTFS Version"
      NTFS Version   :                  3.1
      
      C:\WINDOWS\system32>fsutil 8dot3name query c: | find "volume state"
      The volume state is: 0 (8dot3 name creation is enabled).
      

      Therefore the call GetShortPathNamecontinued to work: the system would look up for the corresponding entry in the file system and return it. The call, of course, breaks if the file system doesn't contain the corresponding 8.3 name entry for a given file anymore. The recent decision to disable the creation of the 8.3 entries was based on the assumption that most of the programs used today don't depend on them anymore.

      The problem with the SDCC code was that it depended on the 8.3 convention even on the systems which were able to cope with the longer names without it, as Windows 95, 98 and ME were the last versions that used the command.com "shell", and the NT-based systems with cmd.exewere already able to work without that. Luckily, the logic behind that decision (always going through GetShortPathName) was also clearly documented in the code.

      This should be enough context needed for reading the report and the comments in [bugs:#3616]

       

      Related

      Bugs: #3616

  • Philipp Klaus Krause

    • Description has changed:

    Diff:

    --- old
    +++ new
    @@ -1,3 +1,3 @@
    -The obvious way to fix #3616 is to remove code that worked around the `command.com`-specific  issues, which shouldn't be relevant anymore as the SDCC Windows binaries already depend on Windows XP or higher version, and the code for `command.com` would only be relevant if they'd run on Windows 95, 98 or ME.
    +The obvious way to fix [bugs:#3616] is to remove code that worked around the `command.com`-specific  issues, which shouldn't be relevant anymore as the SDCC Windows binaries already depend on Windows XP or higher version, and the code for `command.com` would only be relevant if they'd run on Windows 95, 98 or ME.
    
     Attached is a patch that removes the conversion to 8+3 short form of the program's full path and filename on Windows (which was made because of that old `command.com` convention), and instead unconditionally puts the double quotes both around the whole command line, and inside around the program with the path, which would work at least since Windows 2000. This is needed as the program with the path can contain spaces in the path or even the program name, and both mentioned quoting steps are needed for the proper functioning of invocations.
    
    • Group: -->
     

    Related

    Bugs: #3616

  • Maarten Brock

    Maarten Brock - 2024-10-26

    the SDCC Windows binaries already depend on Windows XP or higher version

    Is that an official decision of the team already?
    Or is it just due to the new preprocessor not (yet?) working on older Windows versions?
    What about keep using the GetShortPathName on 32 bit Windows?

     
    • Philipp Klaus Krause

      We might not have made the decision consciously, but we made it, and I don't think it is feasible to support Windows older than Windows XP. Even for Windows XP we should test to see if it actually works.

      • We use langauge standards (https://sourceforge.net/p/sdcc/wiki/Language%20dialects%20used%20in%20SDCC/) resulting in minium requirements on compilers. AFAIK compilers fulfilling these requirement do not support targeting Windows older than XP. On the Microsoft side, Visual Studio 2019, Version 16.7 was the last version to support Windows XP, and I think it is possible to compile SDCC with that, but I'm not sure.
      • The minimum version of the boost library that we require does not support Windows older than XP.
      • uCsim explicitly requires at least Windows 2000 (#define WINVER 0x500 in sim/ucsim/src/core/utils.src/fwio.cc).
       

      Last edit: Philipp Klaus Krause 2024-10-27
      • Maarten Brock

        Maarten Brock - 2024-10-27

        Can it please be documented in the manual then that SDCC requires at least Windows XP?

         
        • Philipp Klaus Krause

          The only reference to older Windows I found in the manual was in a section about configure caching on Cygwin. I just removed that one, since it is IMO, out of the scope of the SDCC manual.
          There is no mention of a minimum Windows version in our manual. Don't know if it makes sense to add one (after all we didn't state that Windows 3.1 is unsupported before either)..

           
    • Janko Stamenović

      I could modify the code to continue calling GetShortPathName if the returned Windows version is "older than Windows 2000" (as the only versions that need GetShortPathName are 95, 98 and ME), but I suspect it wouldn't be so easy to keep maintaining SDCC actually run on these systems. Already published SDCC 4.4, from January 2024, is the first release of SDCC that had a dependency on libstdc++-6.dll, and that .dll already doesn't work on anything older than Windows XP.
      It is because I've observed that 32-bit SDCC already doesn't run on Windows 2000 that I've decided to remove the use of GetShortPathName.

       
      • Benedikt Freisen

        Let's keep in mind that even Microsoft's extended support for Windows XP ended a decade ago. I see no good reason for us to support Windows versions that are even more ancient, particularly if doing so is not trivial.

         
        👍
        1
        • Janko Stamenović

          Windows 95, 98 and ME are by design less stable than any NT-based version, as they were specifically made for the computers with very little memory, for example, just 16 or 32 MB, and they achieved that by not having all memory protection mechanisms. For programming, having a computer with more RAM and using NT 4 had clear benefits even at that time (less chances for a crash or memory corruption), and the benefits continued with Windows 2000 (also NT-based). As the computers became more powerful, and Microsoft did more development and compatibility tweaks (to allow older software to run in spite of the new base), the XP, from 2001, was the first NT-based system for "common users".

          So, I also don't see any benefit of a new 2024 version of compiler to support the unstable 20th-century OS-es which were specially designed for very little RAM, and compatibility with even older software, like 95, 98 and ME.

          On another side, I use (!) or know some people who use Windows 7 and 8.1-based computers, which are also "not supported by Microsoft", so I believe that some such compatibility is useful to many, as I think there are still many computers which are fully functional and with enough RAM and disk, but to which newer Microsoft OS-es refuse to install or simply don't have drivers. They aren't considered "safe" for internet surfing today, but the tasks like compiling aren't that. And luckily compilers don't have some special hardware needs.

           
  • Maarten Brock

    Maarten Brock - 2024-10-26
    +static unsigned                    // FIXME: make this a boolean
    +file_exists(const char* path)
    
    +  unsigned cmd_exists;             // FIXME: here as well
    
    -  /* allocate extra space for ' ' and '\0' */
    -  dbuf_init (&dbuf, strlen (command) + strlen (params) + 2);
    +  /* allocate extra space for '"' '"' ' ' and '\0' */   /* FIXME:
    change to:                     '""' '" ' and '"\0' */
    +  dbuf_init (&dbuf, strlen (command) + strlen (params) + 6);
    
    +  dbuf_append_str (&dbuf, "\"\"");
       dbuf_append_str (&dbuf, command);
    +  dbuf_append_str (&dbuf, "\"");   /* FIXME: combine these two lines */
       dbuf_append (&dbuf, " ", 1);     /* ------------------------------ */
       dbuf_append_str (&dbuf, params);
    +  dbuf_append_str (&dbuf, "\"");
    
     
    👍
    1
    • Janko Stamenović

      I hope I've fixed all here (attached).

       
      • Philipp Klaus Krause

        I've put this patch into [r15082]. Let's see how the results in the compile farm look tomorrow.

         

        Related

        Commit: [r15082]

  • Philipp Klaus Krause

    • status: open --> closed-fixed
    • assigned_to: Philipp Klaus Krause
     
  • Philipp Klaus Krause

    Thanks. This patch has been in trunk for a while, and has been confirmed to fix the bug.

     

Log in to post a comment.

Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.