[PATCH] Fix hardcoded path seperator in two code locations
Yaron Keren
yaron.keren at gmail.com
Fri May 9 07:38:12 PDT 2014
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/7c2077ab/attachment.html>
More information about the llvm-commits
mailing list