zturner (Zachary Turner)
User

Projects

User does not belong to any projects.

User Details

User Since
May 26 2014, 12:49 PM (195 w, 2 d)

Recent Activity

Today

zturner added a comment to D43165: [lit] Fix problem in how Python versions open files with different encodings.

I checked the code and this happens indirectly inside of a call to compareDirectoryTrees. So indeed, it would be hard to explicitly request a binary diff. Instead, could we detect if a file is binary, and if it is fall back to a binary diff? For example, using code like this. We could put this in a function named lit.util.is_binary(file) and inside of compareDirectoryTrees we could call this function and then branch to the appropriate diff function.

Thoughts?

Wed, Feb 21, 1:28 PM
zturner added a comment to D43165: [lit] Fix problem in how Python versions open files with different encodings.

I checked the code and this happens indirectly inside of a call to compareDirectoryTrees. So indeed, it would be hard to explicitly request a binary diff. Instead, could we detect if a file is binary, and if it is fall back to a binary diff? For example, using code like this. We could put this in a function named lit.util.is_binary(file) and inside of compareDirectoryTrees we could call this function and then branch to the appropriate diff function.

Wed, Feb 21, 1:25 PM

Yesterday

zturner accepted D43096: [lit] Update how clang and other binaries are found in per-configuration directories.
Tue, Feb 20, 10:53 AM

Fri, Feb 16

zturner added a comment to D43419: Fix TestBreakpointInGlobalConstructor for Windows.

I guess on the other hand, it's reasonable to assume that if you've set a breakpoint somewhere, you're more likely than not to need it since you probably had a reason for setting it. Is the debug info parsed when the executable is loaded, or when the breakpoint is set?

Fri, Feb 16, 4:38 PM
zturner added a comment to D43419: Fix TestBreakpointInGlobalConstructor for Windows.

I guess one advantage to delaying it is that the debug info for the dynamic library could be large and slow to parse, and you don't know if you're even going to need it until you hit the breakpoint. So by delaying the resolution even to File address, you postpone parsing potentially expensive debug info until you know you're going to need it.

Fri, Feb 16, 4:34 PM
zturner added a comment to D43419: Fix TestBreakpointInGlobalConstructor for Windows.

I'm not sure how hard it would be, but it would be better if we could do the same thing on Windows, for consistency of behavior. In principle it's straightforward, just scan through the IAT and load all of the imported modules. Probably the DynamicLoaderWindows plugin is doing this right now.

Fri, Feb 16, 4:27 PM
zturner added a comment to D43419: Fix TestBreakpointInGlobalConstructor for Windows.

On Darwin we load all the libraries that the binary links against pre-execution, if possible. So I see:

% lldb a.out
(lldb) ima li libfoo.dylib
[ 0] 35C5FE62-5327-3335-BBCF-5369CB07D1D6 0x0000000000000000 /Volumes/ssdhome/jmolenda/k/svn/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/libfoo.dylib

/Volumes/ssdhome/jmolenda/k/svn/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/libfoo.dylib.dSYM/Contents/Resources/DWARF/libfoo.dylib

(lldb) br s -f foo.cpp -p BR_foo
Breakpoint 1: where = libfoo.dylib`Foo::Foo() + 18 at foo.cpp:4, address = 0x0000000000000f52
(lldb) br li
Current breakpoints:
1: source regex = "BR_foo", exact_match = 0, locations = 1

1.1: where = libfoo.dylib`Foo::Foo() + 18 at foo.cpp:4, address = libfoo.dylib[0x0000000000000f52], unresolved, hit count = 0
Fri, Feb 16, 4:17 PM
zturner added a reviewer for D43419: Fix TestBreakpointInGlobalConstructor for Windows: labath.

What's the behavior on other platforms? When you set a breakpoint in a shared library before you've run the program, shouldn't it still be unresolved, in which case this test should have failed on those platforms too?

Fri, Feb 16, 4:03 PM
zturner committed rL325389: Try again to fix the build..
Try again to fix the build.
Fri, Feb 16, 1:13 PM
zturner committed rL325388: Try to fix broken build with some compilers..
Try to fix broken build with some compilers.
Fri, Feb 16, 1:00 PM
zturner committed rL325386: Fix emission of PDB string table..
Fix emission of PDB string table.
Fri, Feb 16, 12:48 PM
zturner closed D43326: [PDB] Fix emission of PDB string table.
Fri, Feb 16, 12:48 PM
zturner added a comment to D43326: [PDB] Fix emission of PDB string table.

Anyone have any comments on this?

Fri, Feb 16, 11:15 AM

Thu, Feb 15

zturner added inline comments to D43326: [PDB] Fix emission of PDB string table.
Thu, Feb 15, 11:41 AM
zturner committed rL325275: Silence warning about unused private variable..
Silence warning about unused private variable.
Thu, Feb 15, 10:49 AM
zturner committed rL325274: Call FlushFileBuffers on output files..
Call FlushFileBuffers on output files.
Thu, Feb 15, 10:39 AM
zturner closed D42925: Call FlushFileBuffers on readwrite file mappings..
Thu, Feb 15, 10:39 AM

Wed, Feb 14

zturner created D43326: [PDB] Fix emission of PDB string table.
Wed, Feb 14, 4:42 PM
zturner accepted D43215: Supply missing break in case statement..
Wed, Feb 14, 3:16 PM
zturner accepted D43165: [lit] Fix problem in how Python versions open files with different encodings.

I don't have a strong opinion, but this seems fine. Feel free to commit and watch the bots to see if any other platforms break.

Wed, Feb 14, 10:39 AM

Tue, Feb 13

zturner added a comment to D43265: [lit] Fix a problem with spaces in the python path by adding quotes around it.

I wonder if it would be worth fixing this in lit? Seems like each substitution should be inherently quoted. Do you know where in lit this is actually failing? I only bring this up because this is the second such bug you've fixed of this nature, so if it's a recurring problem then we should perhaps fix it at the source. For example, what if %clang also has spaces in the path? Will that also fail?

Tue, Feb 13, 3:58 PM
zturner added a reviewer for D43215: Supply missing break in case statement.: asmith.

Aaron, do you remember why you added a check for width == 0 here? Would it ever not be 0?

Tue, Feb 13, 1:47 PM

Mon, Feb 12

zturner added a comment to D43048: [lldb-test/WIP] Allow a way to test autocompletion.

No, the unwind unittests that exist today should stay written as unit tests. These are testing the conversion of native unwind formats -- for instance, eh_frame, compact unwind, or instruction analysis -- into the intermediate UnwindPlan representation in lldb. They are runtime invariant, unit tests are the best approach to these. If there were anything to say about these, it would be that we need more testing here - armv7 (AArch32) into UnwindPlan is not tested. eh_frame and compact_unwind into UnwindPlan is not tested.

That's exactly the type of thing that FileCheck tests work best for. I'm not sure why you're saying that unittests are better than FileCheck tests for this scenario.

Mon, Feb 12, 12:47 PM

Fri, Feb 9

zturner added a comment to D43099: Make LLDB's clang module cache path customizable.

It's probably possible to make it work, but as Jim said, there's no drop in replacement currently. There's bits and pieces of stuff that, with a dedicated effort, could be improved to the point of being sufficient, though.

Fri, Feb 9, 10:39 AM

Thu, Feb 8

zturner added inline comments to D43096: [lit] Update how clang and other binaries are found in per-configuration directories.
Thu, Feb 8, 4:15 PM
zturner accepted D42443: [SymbolFilePDB] Add support for function symbols.

Sorry for the delay on this one, looks good.

Thu, Feb 8, 2:54 PM
zturner accepted D43059: Recognize MSVC style mangling in CPlusPlusLanguage::IsCPPMangledName.

Looks good. Wish this function took a StringRef so you could just say return name.startswith("?") || name.startswith("_Z")

Thu, Feb 8, 2:47 PM
zturner added inline comments to D43060: [CodeView] Lower type for dwarf::DW_TAG_restrict_type type.
Thu, Feb 8, 2:43 PM
zturner added inline comments to D43060: [CodeView] Lower type for dwarf::DW_TAG_restrict_type type.
Thu, Feb 8, 2:40 PM
zturner added a comment to D43060: [CodeView] Lower type for dwarf::DW_TAG_restrict_type type.

(I can review too btw, just would suggest adding him also)

Thu, Feb 8, 2:24 PM
zturner added a reviewer for D43060: [CodeView] Lower type for dwarf::DW_TAG_restrict_type type: rnk.

+rnk, in the future add him on anything MSABI specific.

Thu, Feb 8, 2:12 PM
zturner accepted D42994: Only throw -fPIC when building a shared library.

This lgtm. If this causes some tests that were previously skipped or xfailed to start passing, you can unskip / unxfail them at the same time.

Thu, Feb 8, 1:54 PM
zturner updated the diff for D43002: Emit S_OBJNAME symbol in CodeView.

Modified the tests as suggested by @aprantl

Thu, Feb 8, 11:03 AM
zturner updated the diff for D43002: Emit S_OBJNAME symbol in CodeView.

Adds tests for bitcode upgrade and assembly roundtripping (as well as the necessary fixes to make these tests pass).

Thu, Feb 8, 10:32 AM

Wed, Feb 7

zturner added a comment to D43048: [lldb-test/WIP] Allow a way to test autocompletion.

On the issue of keeping the other test, I think when an SB API method is basically a pass-through to a private method, then having a test of the SB API method that verifies "did the correct native method get called" is useful if for no other reason than to verify the correctness of the SWIG binding generation. If you've tested that the API method invokes the correct native method, and you've tested the native method directly with a large amount of inputs, then that's sufficient overall coverage IMO

Wed, Feb 7, 4:20 PM
zturner added a comment to D43048: [lldb-test/WIP] Allow a way to test autocompletion.

Number of matches might be sufficient, but it might be nice to know if the ordering of matches changes for some reason. Unless there's a specific reason we want to allow an unstable order, enforcing a stable order seems desirable just on principle.

Wed, Feb 7, 3:46 PM
zturner added a comment to D43048: [lldb-test/WIP] Allow a way to test autocompletion.

By the way, I'd suggest printing indices in front of each match and including those in the FileCheck tests. Otherwise we could miss completions that sneak in.

Wed, Feb 7, 3:09 PM
zturner added a comment to D43048: [lldb-test/WIP] Allow a way to test autocompletion.

In the future, we could add options to the autocomplete subcommand that would allow specification of a target, and things like cursor position to maximize testability.

Wed, Feb 7, 3:07 PM
zturner updated the diff for D43002: Emit S_OBJNAME symbol in CodeView.

Gated the passing of this flag behind a check for CodeView. I also removed the O(N^2) function anyway in favor of just calling make_absolute. This seems more correct anyway as it will handle the case where the output file is specified as an absolute path already.

Wed, Feb 7, 2:31 PM
zturner committed rL324504: Generate PDB files for profiling even in Release build..
Generate PDB files for profiling even in Release build.
Wed, Feb 7, 11:40 AM
zturner closed D42632: Generate PDB files for profiling even in Release build.
Wed, Feb 7, 11:40 AM
zturner added a comment to D43002: Emit S_OBJNAME symbol in CodeView.

This depends on whether this is just a nice-to-have feature for Windows or if it is important enough to warrant creating a new API function.

I can back out the api changes, it doesn't seem important enough to me until there is at least 1 active user of the c api who wants it.

Wed, Feb 7, 10:44 AM
zturner added a comment to D43002: Emit S_OBJNAME symbol in CodeView.

It would be more efficient to put, e.g., a NamedMDNode into the module so this information can be shared between the CUs.

I assume the string data itself is shared by the bitcode format? But I don't really know.

Wed, Feb 7, 10:28 AM
zturner added a comment to D43002: Emit S_OBJNAME symbol in CodeView.

Does this need to be the name of an actual object file? Any idea how this should work in the case of LTO?

For the .dwo file name, which must be the actual name of the final .dwo file (ie: intermediate IR 'object' file names are not relevant, only the final true object file because that's where the .dwo file comes from) this wasn't passed down through debug info metadata (because at the time the metadata's generated the final object file name is not known - that metadata might be merged, split, Thin merged, etc, before the final object is emitted) but through the MCOptions struct passed into LLVM's MC layer.

Wed, Feb 7, 9:36 AM

Tue, Feb 6

zturner created D43002: Emit S_OBJNAME symbol in CodeView.
Tue, Feb 6, 7:33 PM
zturner added a comment to D42994: Only throw -fPIC when building a shared library.

In the future when you upload diffs can you include context? (i.e. git diff -U999999). It's nice to be able to see the surrounding code when I'm looking at a diff.

Tue, Feb 6, 5:07 PM
zturner added inline comments to D42762: Rewrite the VS Integration Scripts.
Tue, Feb 6, 11:49 AM

Mon, Feb 5

zturner updated the diff for D42762: Rewrite the VS Integration Scripts.

Added a UI setting that allows the user to choose a custom path to clang. It defaults to $(LLVMInstallDir)bin\clang-cl.exe. I added this to the general page of the UI. I then initialize the value of $(CLToolExe) with the value of this setting in the .targets file. And since I'm mucking with that page anyway, removed some settings that don't apply to clang-cl, surrounding managed code and App Store stuff.

Mon, Feb 5, 4:23 PM
zturner added a comment to D42925: Call FlushFileBuffers on readwrite file mappings..
In D42925#998354, @rnk wrote:

One concern I have is that this is a common pattern on Linux:

int fd = open(...);
void *mem = mmap(fd, ...);
close(fd);
// use mem

Are we reasonably confident that LLVM doesn't have this pattern anywhere in it? i.e. we never store the FD and then try to use it after closing it?

Mon, Feb 5, 3:38 PM
zturner added inline comments to D42925: Call FlushFileBuffers on readwrite file mappings..
Mon, Feb 5, 1:04 PM
zturner added a comment to D42762: Rewrite the VS Integration Scripts.

Question for @olgaark. Is there any way to get a page that looks like PageTemplate="generic" (i.e. with the expanding categories, such as the General page), but as a subpage of a PageTemplate="tool" page? For example, imagine you click Properties > C/C++ > Warnings, and then when you click the warnings page, I'd like that page to look like PageTemplate="generic". Is that possible?

Mon, Feb 5, 1:02 PM
zturner added inline comments to D42762: Rewrite the VS Integration Scripts.
Mon, Feb 5, 12:51 PM
zturner added inline comments to D42762: Rewrite the VS Integration Scripts.
Mon, Feb 5, 12:49 PM
zturner added a comment to D42926: [CodeView] Initial support for emitting S_BLOCK32 symbols for lexical scopes.

Very excited to see someone doing this. We've been meaning to do it for some time but always get sidetracked on other things.

Mon, Feb 5, 12:00 PM
zturner created D42925: Call FlushFileBuffers on readwrite file mappings..
Mon, Feb 5, 11:38 AM
zturner added inline comments to D42762: Rewrite the VS Integration Scripts.
Mon, Feb 5, 11:30 AM
zturner added a comment to D42914: Rewrite TestTargetSymbolsBuildidCase to be more focused.

Yea this seems like a good candidate for an lldb-test test. Something like this.

Mon, Feb 5, 10:51 AM
zturner added inline comments to D42762: Rewrite the VS Integration Scripts.
Mon, Feb 5, 10:00 AM
zturner added inline comments to D42762: Rewrite the VS Integration Scripts.
Mon, Feb 5, 8:40 AM
zturner added inline comments to D42762: Rewrite the VS Integration Scripts.
Mon, Feb 5, 8:09 AM

Fri, Feb 2

zturner abandoned D42769: Create a VSIX Installer for the VS Integration.

This was merged into D42762

Fri, Feb 2, 1:07 PM
zturner updated the diff for D42762: Rewrite the VS Integration Scripts.

Don't install the batch files for real. The previous patch didn't copy them to the output directory on install, but it failed to remove the invocation of install.bat and uninstall.bat from the actual installer.

Fri, Feb 2, 11:51 AM
zturner updated the diff for D42762: Rewrite the VS Integration Scripts.
  1. Removed the option remapping. I will be adding this back in a follow up patch. The clang-cl specific "limited options view" is still here.
  2. Removed the dependency on writing the version number to the registry. This was originally done so that we could add the clang resource dir to the include path, but clang can always figure out its own resource path anyway, so it turns out this was unnecessary. This has the nice property that it will automatically work with all past, present, and future clang versions without modification.
  3. Changed "clang" to "llvm"
  4. Merged D42769 into this patch.
  5. Remove the batch files (and for that matter all other files as well) from being installed by CMake. The vsix is now the installer, which we can publish on the marketplace.
Fri, Feb 2, 11:49 AM

Thu, Feb 1

zturner added inline comments to D42762: Rewrite the VS Integration Scripts.
Thu, Feb 1, 8:48 AM

Wed, Jan 31

zturner updated the diff for D42769: Create a VSIX Installer for the VS Integration.

This update brings a couple of minor tweaks.

Wed, Jan 31, 5:52 PM
zturner updated the diff for D42769: Create a VSIX Installer for the VS Integration.

I made a few minor changes in this update.

Wed, Jan 31, 4:46 PM
zturner created D42769: Create a VSIX Installer for the VS Integration.
Wed, Jan 31, 4:24 PM
zturner added inline comments to D42443: [SymbolFilePDB] Add support for function symbols.
Wed, Jan 31, 1:35 PM
zturner updated the summary of D42762: Rewrite the VS Integration Scripts.
Wed, Jan 31, 1:24 PM
zturner added a reviewer for D42762: Rewrite the VS Integration Scripts: asmith.

+asmith

Wed, Jan 31, 1:21 PM
zturner created D42762: Rewrite the VS Integration Scripts.
Wed, Jan 31, 1:02 PM

Tue, Jan 30

zturner committed rL323790: [CodeView] Micro-optimizations to speed up type merging..
[CodeView] Micro-optimizations to speed up type merging.
Tue, Jan 30, 9:16 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Tue, Jan 30, 9:16 AM

Mon, Jan 29

zturner added a comment to D42656: [testsuite] Remove flakey test relying on `pexpect`.

Spinning up a process just to test that auto-completion works though seems a little bit unnecessary, and introduces the possibility that flakiness and bugs in the process spawn mechanism (if any exist) will manifest in the test failure. It would be nice, if and when this test fails, to have some confidence that the failure is related to auto completing symbol names.

Mon, Jan 29, 3:26 PM
zturner added a comment to D42656: [testsuite] Remove flakey test relying on `pexpect`.

If we just need to test completion, write a lit-style test that uses lldb-test that looks like this:

Mon, Jan 29, 3:15 PM
zturner accepted D42632: Generate PDB files for profiling even in Release build.

True, link time would probably be affected. But I think if anyone is concerned about this, they can add -DLLVM_DISABLE_PDB. Having it generate a PDB should be the default IMO.

Mon, Jan 29, 11:02 AM

Fri, Jan 26

zturner updated the diff for D42559: [CodeView] Speed up type merging by about 20%.

Return an empty record when remapping fails.

Fri, Jan 26, 9:34 AM

Thu, Jan 25

zturner added inline comments to D42559: [CodeView] Speed up type merging by about 20%.
Thu, Jan 25, 3:46 PM
zturner added inline comments to D42559: [CodeView] Speed up type merging by about 20%.
Thu, Jan 25, 3:45 PM
zturner created D42559: [CodeView] Speed up type merging by about 20%.
Thu, Jan 25, 3:07 PM
zturner accepted D42547: [sanitizer] Implement GetNumberOfCPUs for Windows.

lgtm

Thu, Jan 25, 9:37 AM

Tue, Jan 23

zturner accepted D42434: [SymbolFilePDB] Fix null array access when parsing the type of a function without any arguments, i.e. 'int main()' and add support to test it.

looks good. May as well fix the BCD typo while you're here though (either here or in a followup patch)

Tue, Jan 23, 12:26 PM

Jan 21 2018

zturner accepted D42347: Fix memory leaks in MinidumpParserTest.
Jan 21 2018, 12:42 PM
zturner added a comment to D42347: Fix memory leaks in MinidumpParserTest.

Same as with the last one. For obviously correct bug fixes like this, just commit them. As an aside, make_unique will make this a bit shorter so it fits on one line. e.g. auto reg_interface = llvm::make_unique<RegisterContextLinux_x86_x64>(arch);

Jan 21 2018, 12:42 PM
zturner accepted D42345: Make loop counter unsigned in SymbolFilePDB::GetCompileUnitIndex.

Trivial fixes like this don't need to be reviewed, you can just commit them. Thanks for your work btw!

Jan 21 2018, 12:42 PM

Jan 20 2018

zturner added inline comments to D42340: [modules] Fix missing includes/typo in LLDB's includes. [NFC].
Jan 20 2018, 8:19 PM

Jan 19 2018

zturner added a comment to D42326: [COFF] don't replace import library if contents are unchanged.
  1. Enabling the new functionality by default. I did not consider this safe to do, because it breaks the assumptions many people and build systems make about how things work. If A depends on B and B depends on C, then, without the change, changing C will cause A and B to be rebuilt and receive new timestamps. With the change, if rebuilding B results in the same bytes that were already in there, B will retain its old timestamp and A will not be rebuilt. This also means that B will now look older than C. This seems like an unexpected enough change in semantics that I would want people to explicitly opt into it.
Jan 19 2018, 5:09 PM
zturner accepted D41427: [SymbolFilePDB] Fix null array access when parsing the type of a function without any arguments, i.e. 'int main()' and add support to test it.

Sorry for forgetting about this! lgtm

Jan 19 2018, 1:50 PM

Jan 18 2018

zturner added inline comments to D42281: Compile the LLDB tests out-of-tree.
Jan 18 2018, 8:01 PM
zturner committed rLLD322871: Speed up iteration of CodeView record streams..
Speed up iteration of CodeView record streams.
Jan 18 2018, 10:38 AM
zturner committed rL322871: Speed up iteration of CodeView record streams..
Speed up iteration of CodeView record streams.
Jan 18 2018, 10:38 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Jan 18 2018, 10:37 AM

Jan 17 2018

zturner committed rL322736: [coff] Print detailed timing information with /TIME..
[coff] Print detailed timing information with /TIME.
Jan 17 2018, 11:18 AM
zturner committed rLLD322736: [coff] Print detailed timing information with /TIME..
[coff] Print detailed timing information with /TIME.
Jan 17 2018, 11:18 AM
zturner closed D41915: [lldCOFF] Print detailed timing information with /VERBOSE.
Jan 17 2018, 11:18 AM
zturner added a comment to D42182: Add LLDB_LOG_ERROR (?).

In a way it's kind of built into the semantics of llvm::Error that this is the only way it could possibly work, since it's a move only type. If you do this, for example:

Jan 17 2018, 10:37 AM

Jan 16 2018

zturner added a comment to D42094: Specify inline for isWhitespace in CommandLine.cpp.
In D42094#977730, @ruiu wrote:

I tried that and you are right. If you pass LLVM_BUILD_TYPE=RelWithDebInfo, it passes /Ob1 which disables function inlining except for functions that explicitly marked "inline". That logic is not in our configuration. CMake passes /Ob1.

Looks like we shouldn't use RelWithDebInfo at all when doing profiling. Instead we should always pass LLVM_BUILD_TYPE=Release with CMAKE_C{,XX}_FLAGS=/Zi.

Jan 16 2018, 5:12 PM
zturner created D42148: [CodeView] Speed up type iteration.
Jan 16 2018, 4:42 PM
zturner accepted D41801: Fix pretty printing the unspecified param of a variadic function.

LGTM, sorry for the delay, thanks for reminding me.

Jan 16 2018, 3:11 PM
zturner accepted D42125: [CodeView] Allow variable names to be as long as the codeview format supports.
Jan 16 2018, 1:29 PM