- User Since
- Jan 26 2015, 9:26 AM (212 w, 2 d)
Mon, Feb 11
My knowledge of PECOFF is even more limited, but it's my understanding that the ImageBase is the _preferred_ address for the module. That doesn't guarantee that's the actual address it would be loaded at. Not just because of ASLR but also because there may be conflicts with modules that have already been loaded. Is GetBaseAddress supposed to return the actual base address or the preferred one?
Fri, Feb 8
I think absolute_path is great. Thanks for checking on the native/None. I didn't want there to be any latent surprises.
Thu, Feb 7
Tue, Feb 5
Jan 10 2019
Jan 9 2019
So this Patch is effectively NFC, since no caller (not even a test) was using the functionality you've removed. Seems like a nice refactor.
Jan 8 2019
Dec 6 2018
If there is a magic value, should we be checking it?
Nov 30 2018
This looks good to me, save for a couple comment nits.
Jul 19 2018
Jul 12 2018
LGTM if you don't want to consider my remaining suggestion for this patch.
Jul 11 2018
Also, I'm not seeing tests for the other consistency checks you're adding (like whether there are any streams or whether the streams overlap). Is it difficult to create such malformed minidumps?
Jul 10 2018
Jul 9 2018
Jul 3 2018
Will making it public cause more TUs to transitively include <windows.h>, or does that happen anyway?
Jun 22 2018
I mostly agree with Zach. In practice, setting core.autocrlf to false on Windows seems the most practical solution.
Jun 13 2018
My motivation was not to check for different kinds of errors but to ensure that the error code detail doesn't get lost through the SWIG plumbing. SBErrors seem to be marshaled properly when used as return values, but this is the first one I've seen returned by reference. The test ensures that the success/fail result makes it, but not that the detail (e.g., the error string) survives the trip.
Jun 4 2018
I like the direction. Just a question about the implicit type conversion.
May 31 2018
I was under the impression that some tools rely on the fact that arg is always expanded to an absolute path. Does this work with lldb and its test suite?
May 15 2018
May 14 2018
Thanks for splitting the test.
I kinda wish the test was broken up a bit. The stringtable bundle logic is distinctly different than just applying the keywords to other resource types, so that would be a natural place to break this apart.
Ah, I see you figured out the discrepancy in a follow-on patch (46816), so this LGTM.
May 9 2018
LGTM, but consider inline suggestion.
The docs suggest there are differences between RCDATA and user-defined resource definitions, but the Microsoft rc.exe doesn't seem to follow the docs. For example, the docs say user-defined types can use an external file but that RCDATA cannot. In real life, the resource compiler will accept an RCDATA statement that does reference an external file. Also, the docs say RCDATA can have VERSION, CHARACTERISTICS, and LANGUAGE optional statements, but those don't seem to work because the keywords are mistaken for file names.
May 8 2018
May 7 2018
I'm happy. Thanks!
As I recall, commas may be optionally allowed in several places besides string tables. We'll have to check that and, if necessary, address in a subsequent patch.
May 2 2018
I'm mildly concerned about the tests that give filenames with forward slashes. That doesn't work with a lot of Windows command lines, but apparently the tests here were already doing that.
Apr 30 2018
The big picture here is fine.
Looks good to me now, but I'm not an official reviewer.
Apr 27 2018
This looks fine to me. I just dropped in to add some context on MSVC and __cplusplus: https://blogs.msdn.microsoft.com/vcblog/2018/04/09/msvc-now-correctly-reports-__cplusplus/
Apr 19 2018
Apr 18 2018
I actually preferred the factory solution.
Apr 17 2018
LGTM, but consider highlighting the side effect to target when the factory makes a Placeholder module.
Apr 16 2018
Apr 13 2018
I meant to mention that the instance of the empty class is reported as 8 bytes of padding (in a 64-bit build), in case that's relevant to anybody's understanding of the issue.
Apr 9 2018
Yes, by all means. My intent was not to block this but to point out that it might still suffer from an already existent problem.
This looks like an improvement, but it still might not work on Windows, because there you're racing the file system--deleting a file on Windows is not an atomic, instantaneous operation. Tune in to https://www.youtube.com/watch?v=uhRWMGBjlO8 at 7:30 for details.
Mar 22 2018
Feb 26 2018
Abandoning. This isn't really working on Windows. The breakpoint fails to set for reasons I don't understand yet.
Nice catch. I'll take a closer look to see what exactly is happening on Windows.
Feb 23 2018
Nit: As a single word, "cleanup" is a noun (or an adjective). As two words, "clean up" is a verb. Given that, I'd expect to see classes and objects with names like Cleanup or cleanup and functions to contain CleanUp. When I see an identifier like CleanUpTest, I expect it's a function that cleans up after a test, not a test of a class called CleanUp.
Feb 22 2018
call_foo1() is defined in foo.cpp, which is built into a DLL, and thus needs the __declspec(dllimport/dllexport) from the LLDB_TEST_API. foo.h defines the API for the DLL.
Right now, we have to add an .exe suffix when using this switch, like -fuse-ld=lld.exe.
This must be new-ish code, since I've routinely used -fuse-ld=lld (without .exe) on Windows.
Feb 21 2018
Please ignore. I'm still trying to figure out arc.