[PATCH] Fix hardcoded path seperator in two code locations

Yaron Keren yaron.keren at gmail.com
Fri May 9 07:42:21 PDT 2014


Alp, also look at    http://reviews.llvm.org/D3686

There, TmpDir is in host conventions and the hardcoded '/' results in paths
like

 c:\my\dir/file.cpp

I tried to add you as reviewer there but Phabricator does not know you.


2014-05-09 17:38 GMT+03:00 Yaron Keren <yaron.keren at gmail.com>:

> Hi,
>
> The use case would be building Linux executable on Windows and debugging
> with Linux gdb - but the paths are all '\\'.
>
> This problem should exist in trunk clang as only the last path seperator
> is hardcoded to '/' and the directory before follows host conventions.Try
>
> clang -S -g -target i686-pc-windows-gnu a.c
>
> for some trivial program and look at DW_AT_comp_dir, it will be in host
> convention.
>
> The purpose of the patch is to make alll path seperators in a
> directory+filename consistent. It does not make sense to mix host
> conventions with hardcoded '/'.
>
> We can fix the inconsistency either by always normalizing MCDwarfDirs (and
> elsewhere needed) to '/' or by following the host convention all the time.
> Since most of clang already follows host convention (by using
> llvm::sys::path) including most of the debug info I thought it would make
> sense to fix the few hardcoded path seperators but certainly we
> can normalize all debug info to '/'.
>
> Yaron
>
>
>
>
> 2014-05-09 15:30 GMT+03:00 Alp Toker <alp at nuanti.com>:
>
>>
>> On 09/05/2014 12:48, Yaron Keren wrote:
>>
>>> Hi Takumi,
>>>
>>> I wish DOS would have used '/' but now it's far too late. Even today it
>>> sometimes causes troubles building on Windows, problems with configure,
>>> make,...
>>>
>>> The current state in incosistent, where most of LLVM uses sys::path and
>>> thus '\\' (on WIndows) but some places use hardcodes '/'.
>>>
>>> This patch does not make the '\\' or '/' decision but rather centralizes
>>> the decision to llvm::sys::path, so if someday we would like to switch all
>>> to '/' as you like, only path.cpp would be modified.
>>>
>>
>> Takumi is pointing out that the decision of what path scheme to use on
>> the host is potentially different to the scheme that will be used by the
>> debugger on the target system.
>>
>> With your change Windows binaries will always have Unix paths (unless the
>> compiler itself happens to be running on Windows, in which case Unix
>> binaries will have Windows backslashes) -- does that work in practice with
>> the respective debuggers on those platforms?
>>
>> If so then I don't see a problem.
>>
>> Alp.
>>
>>
>>
>>
>>> Yaron
>>>
>>>
>>>
>>> 2014-05-09 14:39 GMT+03:00 NAKAMURA Takumi <geek4civic at gmail.com<mailto:
>>> geek4civic at gmail.com>>:
>>>
>>>
>>>     I prefer '/' could be used everywhere, unless it were illegal. I
>>> think
>>>     '\\' makes the world worse.
>>>
>>>     2014-05-09 19:53 GMT+09:00 Yaron Keren <yaron.keren at gmail.com
>>>     <mailto:yaron.keren at gmail.com>>:
>>>
>>>     > In the discussion linked, Reid mentioned that when
>>>     cross-compiling aren't
>>>     > the paths are valid on the host only, so shouldn't the host path
>>>     seperator
>>>     > be used? the target may not even have a file system.
>>>     >
>>>     > The alternative is to have all path seperators as '/', but this
>>>     disagrees
>>>     > with path.cpp functionality and all its many clients.
>>>     >
>>>     > Alp, is that whan you meant?
>>>     >
>>>     >
>>>     > 2014-05-09 13:21 GMT+03:00 Alp Toker <alp at nuanti.com
>>>     <mailto:alp at nuanti.com>>:
>>>
>>>     >>
>>>     >>
>>>     >> On 09/05/2014 10:32, Yaron Keren wrote:
>>>     >>>
>>>     >>> Hi echristo, rnk, chapuni,
>>>     >>>
>>>     >>> Earlier duscussion:
>>>     >>> https://www.mail-archive.com/cfe-commits@cs.uiuc.edu/
>>> msg92891.html
>>>     >>>
>>>     >>> Most of LLVM code uses the llvm::sys::path::append function to
>>>     >>> concatenate paths and will get the correct native slash. In
>>>     some code
>>>     >>> locations it seems that direct string concatenation was used,
>>>     possibly
>>>     >>> because the code writer knows there are no extra path
>>>     sepeartors to deal
>>>     >>> with, so string concat was more efficient than append.
>>>     >>>
>>>     >>> In the locations below, the path seperator is hardcoded as
>>>     '/'. Fixed by
>>>     >>> exposing the native seperator from llvm::sys:;path and using it.
>>>     >>>
>>>     >>> As an alternative, the append function could be used but it's
>>>     more code
>>>     >>> and less efficient.
>>>     >>>
>>>     >>> http://reviews.llvm.org/D3687
>>>     >>>
>>>     >>> Files:
>>>     >>>    include/llvm/Support/Path.h
>>>     >>>    lib/MC/MCDwarf.cpp
>>>     >>>    lib/Support/Path.cpp
>>>     >>>    lib/Support/SourceMgr.cpp
>>>     >>>
>>>     >>> Index: include/llvm/Support/Path.h
>>>     >>>
>>>     ===================================================================
>>>     >>> --- include/llvm/Support/Path.h
>>>     >>> +++ include/llvm/Support/Path.h
>>>     >>> @@ -295,6 +295,11 @@
>>>     >>>   /// @result true if \a value is a path separator character
>>>     on the host
>>>     >>> OS
>>>     >>>   bool is_separator(char value);
>>>     >>>   +/// @brief Return the preferred separator for this platform.
>>>     >>> +///
>>>     >>> +/// @result const char* to the preferred separator,
>>>     null-terminated.
>>>     >>> +const char *get_separator();
>>>     >>> +
>>>     >>>   /// @brief Get the typical temporary directory for the
>>>     system, e.g.,
>>>     >>>   /// "/var/tmp" or "C:/TEMP"
>>>     >>>   ///
>>>     >>> Index: lib/Support/Path.cpp
>>>     >>>
>>>     ===================================================================
>>>     >>> --- lib/Support/Path.cpp
>>>     >>> +++ lib/Support/Path.cpp
>>>     >>> @@ -569,6 +569,12 @@
>>>     >>>     }
>>>     >>>   }
>>>     >>>   +static const char preferred_separator_string[] = {
>>>     >>> preferred_separator, '\0' };
>>>     >>> +
>>>     >>> +const char *get_separator() {
>>>     >>> +  return preferred_separator_string;
>>>     >>> +}
>>>     >>> +
>>>     >>>   void system_temp_directory(bool erasedOnReboot,
>>>     SmallVectorImpl<char>
>>>     >>> &result) {
>>>     >>>     result.clear();
>>>     >>>   Index: lib/MC/MCDwarf.cpp
>>>     >>>
>>>     ===================================================================
>>>     >>> --- lib/MC/MCDwarf.cpp
>>>     >>> +++ lib/MC/MCDwarf.cpp
>>>     >>> @@ -689,7 +689,7 @@
>>>     >>>     const SmallVectorImpl<std::string> &MCDwarfDirs =
>>>     >>> context.getMCDwarfDirs();
>>>     >>>     if (MCDwarfDirs.size() > 0) {
>>>     >>>       MCOS->EmitBytes(MCDwarfDirs[0]);
>>>     >>> -    MCOS->EmitBytes("/");
>>>     >>> +  MCOS->EmitBytes(sys::path::get_separator());
>>>     >>>     }
>>>     >>>     const SmallVectorImpl<MCDwarfFile> &MCDwarfFiles =
>>>     >>> MCOS->getContext().getMCDwarfFiles();
>>>     >>
>>>     >>
>>>     >> I know there isn't much clarity yet on the issue but it's worth
>>>     mentioning
>>>     >> (in the commit message or a comment) the current state of
>>>     affairs regarding
>>>     >> paths in cross-compiled debug info.
>>>     >>
>>>     >> Alp.
>>>     >>
>>>     >>
>>>     >>
>>>     >>> Index: lib/Support/SourceMgr.cpp
>>>     >>>
>>>     ===================================================================
>>>     >>> --- lib/Support/SourceMgr.cpp
>>>     >>> +++ lib/Support/SourceMgr.cpp
>>>     >>> @@ -18,6 +18,7 @@
>>>     >>>   #include "llvm/ADT/Twine.h"
>>>     >>>   #include "llvm/Support/Locale.h"
>>>     >>>   #include "llvm/Support/MemoryBuffer.h"
>>>     >>> +#include "llvm/Support/Path.h"
>>>     >>>   #include "llvm/Support/raw_ostream.h"
>>>     >>>   #include "llvm/Support/system_error.h"
>>>     >>>   using namespace llvm;
>>>     >>> @@ -60,7 +61,7 @@
>>>     >>>       // If the file didn't exist directly, see if it's in an
>>>     include
>>>     >>> path.
>>>     >>>     for (unsigned i = 0, e = IncludeDirectories.size(); i != e &&
>>>     >>> !NewBuf; ++i) {
>>>     >>> -    IncludedFile = IncludeDirectories[i] + "/" + Filename;
>>>     >>> +    IncludedFile = IncludeDirectories[i] +
>>>     sys::path::get_separator() +
>>>     >>> Filename;
>>>     >>> MemoryBuffer::getFile(IncludedFile.c_str(), NewBuf);
>>>     >>>     }
>>>     >>>
>>>     >>>
>>>     >>> _______________________________________________
>>>     >>> llvm-commits mailing list
>>>     >>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>>
>>>     >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>     >>
>>>     >>
>>>     >> --
>>>     >> http://www.nuanti.com
>>>     >> the browser experts
>>>     >>
>>>     >
>>>
>>>
>>>
>> --
>> http://www.nuanti.com
>> the browser experts
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140509/0d8e0e4e/attachment.html>


More information about the llvm-commits mailing list