[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