<div dir="ltr">OK, already did the dirent and <span style="font-family:arial,sans-serif;font-size:13px">FileSpec::EnumerateDirectory() things.</span><div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">However, for </span><span style="font-family:arial,sans-serif;font-size:13px">source/Host/common/Host.cpp, it seems easy to move things around.</span></div><div>
<span style="font-family:arial,sans-serif;font-size:13px">However there will still be a single big #ifndef _WIN32 around all those functions since they will still be shared between linux, freebsd and macosx. Is this OK?</span></div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Aug 16, 2013 at 10:38 AM, Greg Clayton <span dir="ltr"><<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Things I would like to see fixed:<br>
<br>
- Do we really need the “include/lldb/Host/windows/dirent.h”? "source/Commands/CommandCompletions.cpp” includes it but it shouldn’t. It CommandCompletions is using diropen and the other dirent calls, it should be switched over to use FileSpec::EnumerateDirectory().<br>
- FileSpec::EnumerateDirectory() should use native Windows directory enumeration instead of the dirent.h compatibility layer if possible<br>
- All code in "source/Host/common/Host.cpp” that us doing "#ifdef _WIN32” inside functions should be copied over into "source/Host/Win32/Host.cpp”. The copied functions should then have only the win32 code, and the code in "source/Host/common/Host.cpp” should remove all references to Win32.<br>
<br>
Let me know if we can get rid of “dirent.h” and “dirent.cpp”, and use a native windows directory enumeration to back FileSpec::EnumerateDirectory(). This will really clean up the code in LLDB to not use native unix code in the core of LLDB and always use the “lldb_private::Host” layer to do all host specific things.<br>
<span class="HOEnZb"><font color="#888888"><br>
Greg<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
<br>
On Aug 14, 2013, at 7:47 AM, Joăo Matos <<a href="mailto:ripzonetriton@gmail.com">ripzonetriton@gmail.com</a>> wrote:<br>
<br>
> Did Greg already approve this?<br>
><br>
><br>
> On Wed, Aug 14, 2013 at 2:42 AM, Virgile Bello <<a href="mailto:virgile.bello@gmail.com">virgile.bello@gmail.com</a>> wrote:<br>
> Thanks for the reviews!<br>
> Please feel free to commit it to trunk (no SVN access here).<br>
><br>
><br>
> On Wed, Aug 14, 2013 at 5:56 AM, Carlo Kok <<a href="mailto:ck@remobjects.com">ck@remobjects.com</a>> wrote:<br>
> Op 8-8-2013 17:03, Malea, Daniel schreef:<br>
><br>
> Hi Virgile, I looked at the Cmake changes and they seem OK. There don't appear to be any regressions on Linux with your patch applied..<br>
><br>
> Cool. Looks good to me too.<br>
><br>
><br>
> Dan<br>
><br>
> From: Virgile Bello <<a href="mailto:virgile.bello@gmail.com">virgile.bello@gmail.com</a><mailto:<a href="mailto:virgile.bello@gmail.com">virgile.bello@gmail.com</a>>><br>
><br>
> Date: Wednesday, 7 August, 2013 1:01 PM<br>
> To: Enrico Granata <<a href="mailto:egranata@apple.com">egranata@apple.com</a><mailto:<a href="mailto:egranata@apple.com">egranata@apple.com</a>>><br>
> Cc: "<a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a><mailto:<a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a>>" <<a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a><mailto:<a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a>>><br>
><br>
> Subject: Re: [Lldb-commits] MinGW compilation support<br>
><br>
> Rebased on latest master, with the regex fix, in case it helps.<br>
> Any chance this could get reviewed/merged please?<br>
><br>
><br>
><br>
> On Mon, Jul 29, 2013 at 11:47 PM, Virgile Bello <<a href="mailto:virgile.bello@gmail.com">virgile.bello@gmail.com</a><mailto:<a href="mailto:virgile.bello@gmail.com">virgile.bello@gmail.com</a>>> wrote:<br>
> Sure, I will put the #ifdef in NSDate formatters for now.<br>
> Also, just noticed a small error, in DisassemblerLLVMC.cpp, I reversed a if:<br>
> if (!s_regex.Execute(out_string, &matches))<br>
> should be<br>
> if (s_regex.Execute(out_string, &matches))<br>
><br>
> Otherwise, anybody had time to take a look at the other parts?<br>
><br>
><br>
> On Mon, Jul 22, 2013 at 10:37 AM, Enrico Granata <<a href="mailto:egranata@apple.com">egranata@apple.com</a><mailto:<a href="mailto:egranata@apple.com">egranata@apple.com</a>>> wrote:<br>
> All that GetOSXEpoch() enables is getting the right summary string for NSDate objects - whether that is or is not critical entirely depends on how much you care about that specific data type having a summary :) If I were to guess, I would say it's pretty minor and can wait for a follow-up patch.<br>
><br>
> If you are going down the route of disabling it, you might also want to disable the NSDate formatter (in Cocoa.cpp) by making<br>
> #ifdef WIN32<br>
> return ""<br>
> #else<br>
> do your thing<br>
> #endif<br>
> That way you will just not get a summary for NSDate on Windows, instead of seeing the wrong date! It seems the safest option to me for now.<br>
><br>
> Once things are stable, we might want to look into Host/TimeValue.cpp to see if it fits the bill, or it should be improved/changed and then port this code over to Host/TimeValue.cpp, but that's for later.<br>
><br>
><br>
> On 07/21/13, Virgile Bello <<a href="mailto:virgile.bello@gmail.com">virgile.bello@gmail.com</a><mailto:<a href="mailto:virgile.bello@gmail.com">virgile.bello@gmail.com</a>>> wrote:<br>
> Yes sure that's what I was planning to do for GetOSXEpoch (using #ifdef for now, it doesn't need to move into Host right away IMHO).<br>
> It should be easy enough to do with Win32 API, and I was planning to do it shortly after in another patch, this one focusing mainly on getting everything to at least compile<br>
> (so I don't spend too much time rebasing this big one, and I can focus on such smaller tasks in individual patches right after, easier to review/integrate individually -- actually there are many things that I improved in otehr patches after actually trying to use LLDB on windows).<br>
><br>
> Do you want me to do an implementation for this patch or you think it is OK for a later patch?<br>
> I agree it might be dangerous to not do an implementation right now since it returns 0 instead of a proper error/assert, so maybe it would be better to implement something for the first version (it didn't seem to be used in important part of the code though)<br>
><br>
><br>
> On Mon, Jul 22, 2013 at 5:47 AM, Enrico Granata <<a href="mailto:egranata@apple.com">egranata@apple.com</a><mailto:<a href="mailto:egranata@apple.com">egranata@apple.com</a>>> wrote:<br>
> Regarding GetOSXEpoch() specifically, while it is a fair point that POSIXy stuff should be moved to the Host layer, and I should spend some time refactoring my (albeit very small) need for time management functions behind something that is more portable, please bear in mind that all that the function does is compute the time_t value for midnight on January 1st 2001. This is because POSIX APIs depend on January 1st 1970 as the "epoch", but Cocoa instead chooses January 1st 2001.<br>
><br>
> If Windows has time_t, you might get away with<br>
> #ifdef _WIN32<br>
> return <hardcoded><br>
> #else<br>
> do all the POSIXy stuff<br>
> #endif<br>
><br>
> With the above said, refactoring my usage of POSIX into the Host layer is definitely a chunk of work that is on me. Once that is done, you should be able to just port the Host's time interface to Win32 vs. POSIX<br>
><br>
> It has been a long time since I did any Windows development (and even then I was mostly using .net instead of Win32), so I am not sure what level of time management API Windows offers, and how close they are to the POSIX facilities. Those who have a better knowledge of that story, feel free to chime in.<br>
><br>
><br>
> On 07/21/13, Virgile Bello <<a href="mailto:virgile.bello@gmail.com">virgile.bello@gmail.com</a><mailto:<a href="mailto:virgile.bello@gmail.com">virgile.bello@gmail.com</a>>> wrote:<br>
> Improved all the things previously discussed:<br>
> - Moved windows specific headers in Host/windows (and including any API/SB*.h header won't include <windows.h>). Probably better than inlining it in lldb-*.h since it shouldn't be visible to external code (i.e. through lldb-defines.h) and shouldn't automatically be added everywhere (through lldb-private*.h). The .cpp files requiring the windows specific API should include those directly since that's where it is used.<br>
> - Concerning LLDB_DISABLE_POSIX, it's done. However, on Windows, any library including LLDB headers (such as API/SB*.h) would need to define it as well. To improve the situation, I added a #define inside Host/mingw/Config.h and added it at the top of every file checking for LLDB_DISABLE_POSIX, so that both LLDB compilation and external project including it don't need additional configuration.<br>
> (Maybe same thing should be done for LLDB_DISABLE_PYTHON on Win32?).<br>
> - Host::Kill added.<br>
> - GetOSXEpoch() still need to be fixed (timegm doesn't exist on Windows). I was planning to fix that in a future patch (doing thing incrementally).<br>
><br>
> It should be a better base for discussion, but might still need some additional improvements and testings.<br>
><br>
><br>
> On Thu, Jul 18, 2013 at 3:36 AM, Greg Clayton <<a href="mailto:gclayton@apple.com">gclayton@apple.com</a><mailto:<a href="mailto:gclayton@apple.com">gclayton@apple.com</a>>> wrote:<br>
> A few issues:<br>
><br>
> Where is _POSIX_SOURCE supposed to come from? I don't see anything that defines this in any makefiles, and it doesn't get defined on darwin which causes compilation problems.<br>
><br>
> We currently assume posix support in LLDB, so I would rather turn these defines into something that needs to be defined to opt out of posix support like:<br>
><br>
> #ifndef DISABLE_POSIX_SUPPORT<br>
><br>
> #endif<br>
><br>
> This way, most unix builds will continue to work without any intervention, and the Windows and MinGW builds will need to make sure DISABLE_POSIX_SUPPORT is defined so all POSIX stuff gets disabled.<br>
><br>
> This patch is also missing the point of the "include/Host" and "source/Host" code. There are many places where a new "lldb/lldb-*.h" file was added, that is essentially doing the job of what the code in "include/Host" and "source/Host" should be doing.<br>
><br>
> Anything that works differently on different platforms should be in the host layer, not in header files that are in "lldb/*.h". A few examples that need to be fixed:<br>
><br>
> RegularExpression.h currently has:<br>
><br>
> #include "lldb/lldb-regex.h"<br>
><br>
> This all of code from that header file:<br>
><br>
> #ifdef _WIN32<br>
> #include "../lib/Support/regex_impl.h"<br>
><br>
> typedef llvm_regmatch_t regmatch_t;<br>
> typedef llvm_regex_t regex_t;<br>
><br>
> inline int regcomp(llvm_regex_t * a, const char *b, int c)<br>
> {<br>
> return llvm_regcomp(a, b, c);<br>
> }<br>
><br>
> inline size_t regerror(int a, const llvm_regex_t *b, char *c, size_t d)<br>
> {<br>
> return llvm_regerror(a, b, c, d);<br>
> }<br>
><br>
> inline int regexec(const llvm_regex_t * a, const char * b, size_t c,<br>
> llvm_regmatch_t d[], int e)<br>
> {<br>
> return llvm_regexec(a,b,c,d,e);<br>
> }<br>
><br>
> inline void regfree(llvm_regex_t * a)<br>
> {<br>
> llvm_regfree(a);<br>
> }<br>
><br>
> #else<br>
> #include <regex.h><br>
> #endif<br>
><br>
><br>
><br>
> Should just be done in "RegularExpression.h". We then need to fix DisassemblerLLVMC.cpp to use the RegularExpression class and not try to use regex directly.<br>
><br>
> We shouldn't need "lldb-windows.h" or "lldb-win32.h", it should be just inlined into "lldb-private.h", or each one should be split up to fit into one of:<br>
><br>
> #include "lldb/lldb-defines.h"<br>
> #include "lldb/lldb-enumerations.h"<br>
> #include "lldb/lldb-forward.h"<br>
> #include "lldb/lldb-types.h"<br>
><br>
> "lldb-socket.h" doesn't really need to exist and can be inlined into the two source files that currently include it. There are so many socket header files and each source file only requires a few of these things, so making one generic header file that includes all socket related headers doesn't make sense.<br>
><br>
> "lldb-dirent.h" is not needed, this should be inlined into FileSpec.cpp and all LLDB code that enumerates directories should use FileSpec::EnumerateDirectory(...).<br>
><br>
> lldb_private::formatters::GetOSXEpoch () disables the function using #ifndef _WIN32, shouldn't it be using DISABLE_POSIX_SUPPORT?<br>
><br>
> Seems like LLDB current uses int kill(pid_t pid, int signo)" in code like ProcessGDBRemote::KillDebugserverProcess() and other places. This should be moved into the Host layer so we do something like:<br>
><br>
> Host::Kill (lldb::pid_t pid, int signo);<br>
><br>
> Then each platform should be able to translate a unix signal into an appropriate kill (TerminateProcess is how windows does this).<br>
><br>
><br>
> So if you can fix these issues and resubmit a patch, I will take a look. I did verify that changing over to using "#ifndef DISABLE_POSIX_SUPPORT" that the project does build on MacOSX.<br>
><br>
><br>
> Greg<br>
> On Jul 13, 2013, at 8:30 AM, Virgile Bello <<a href="mailto:virgile.bello@gmail.com">virgile.bello@gmail.com</a><mailto:<a href="mailto:virgile.bello@gmail.com">virgile.bello@gmail.com</a>>> wrote:<br>
><br>
> Sorry, quick patch update to fix DataBufferMemoryMap::MemoryMapFromFileDescriptor.<br>
><br>
><br>
> On Sat, Jul 13, 2013 at 12:24 PM, Virgile Bello <<a href="mailto:virgile.bello@gmail.com">virgile.bello@gmail.com</a><mailto:<a href="mailto:virgile.bello@gmail.com">virgile.bello@gmail.com</a>>> wrote:<br>
> Ok, attached the updated patch.<br>
> It improves quite a few things:<br>
> - readded some libraries such as PluginObjectContainerBSDArchive and SymbolVendorELF<br>
> - added missing functions in Windows.cpp<br>
> - full support for cmake under MingW.<br>
><br>
> Note that for automake build, you need to add std=c++11 and -DLLDB_DISABLE_PYTHON (automatic for cmake build).<br>
><br>
><br>
> On Thu, Jul 11, 2013 at 10:02 AM, Virgile Bello <<a href="mailto:virgile.bello@gmail.com">virgile.bello@gmail.com</a><mailto:<a href="mailto:virgile.bello@gmail.com">virgile.bello@gmail.com</a>>> wrote:<br>
><br>
> On Thu, Jul 11, 2013 at 4:18 AM, Joăo Matos <<a href="mailto:ripzonetriton@gmail.com">ripzonetriton@gmail.com</a><mailto:<a href="mailto:ripzonetriton@gmail.com">ripzonetriton@gmail.com</a>>> wrote:<br>
> On Sun, Jul 7, 2013 at 10:44 AM, Virgile Bello <<a href="mailto:virgile.bello@gmail.com">virgile.bello@gmail.com</a><mailto:<a href="mailto:virgile.bello@gmail.com">virgile.bello@gmail.com</a>>> wrote:<br>
> Here is a patch allowing compilation of LLDB with MinGW.<br>
> (it still compiles on Linux after the patch)<br>
> Can someone please review (and merge in trunk if everything seems good)?<br>
><br>
> Soon some other patches should follow:<br>
> - 1 for MSVC compilation<br>
> - 1 to add a lldbProcessWindows using Win32 debugging API, so that gdb/clang compiled programs can be debugged in Windows.<br>
><br>
> Hopefully I can get all this into trunk quickly so that I don't need to rebase/merge too much.<br>
> Thanks!<br>
><br>
> Just looked at the patch and it mostly LGTM. I think long-term it would be nice to have the POSIX-specific stuff a little better abstracted, instead of trying to hack around it in the Windows port, though for now it seems the way forward and it can be fixed incrementally in the future.<br>
> I agree. I tried to keep the changes as small as possible (my priority was that it would be easy to integrate into trunk) but long-term it might require some more abstraction.<br>
><br>
><br>
> Some questions:<br>
><br>
> 1. Why do we need to define these in "lldb-socket.h"?<br>
><br>
> +#define NTDDI_VERSION NTDDI_VISTA<br>
> +#define _WIN32_WINNT _WIN32_WINNT_VISTA<br>
><br>
> Can't you just include "lldb-windows.h" which already includes Windows.h?<br>
> You're totally right.<br>
> Actually I thought that was in the MinGW patch but I did right after in the MSVC branch.<br>
> I will backport it and improve this patch a little bit then, in order to avoid further changes later.<br>
><br>
><br>
> 2. Why are you excluding lldbPluginObjectContainerBSDArchive and lldbPluginSymbolVendorELF from the MinGW build?<br>
> No ar.h for lldbPluginObjectContainerBSDArchive (same as before, I patched it in MSVC branch so I should probably backport that as well).<br>
> For lldbPluginSymbolVendorELF I will double-check.<br>
><br>
> I will soon get back to you with an updated patch covering 1 and 2.<br>
><br>
><br>
> 3. Why add support for two build systems (CMake and automake)?<br>
><br>
> IIRC LLVM and Clang are looking to kill all non-CMake build scripts to ease maintenance and reduce duplication, so IMHO LLDB being an umbrella project should do the same.<br>
> I see, so I don't need to support Automake makefile anymore? Good to know. Actually I was still using it for the MingW build (and CMake for MSVC build), but never tested CMake + MingW. I have to give it a try and check everything works fine, esp. if it is the future-proof way.<br>
><br>
><br>
> Apart from that, looking forward to see this getting in and for further Windows debugging patches.<br>
><br>
> --<br>
> Joăo Matos<br>
><br>
><br>
><br>
> <lldb-mingw32-v3.diff>_______________________________________________<br>
> lldb-commits mailing list<br>
> <a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a><mailto:<a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a>><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br>
><br>
><br>
><br>
><br>
><br>
><br>
> _______________________________________________<br>
> lldb-commits mailing list<br>
> <a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br>
><br>
><br>
> _______________________________________________<br>
> lldb-commits mailing list<br>
> <a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br>
><br>
><br>
> _______________________________________________<br>
> lldb-commits mailing list<br>
> <a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br>
><br>
><br>
><br>
><br>
> --<br>
> Joăo Matos<br>
> _______________________________________________<br>
> lldb-commits mailing list<br>
> <a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br>
<br>
</div></div></blockquote></div><br></div>