Projects

User does not belong to any projects.

User Details

User Since
May 26 2014, 12:49 PM (247 w, 4 d)

Yesterday

Fri, Feb 22, 9:17 AM · Restricted Project, Restricted Project

Thu, Feb 21

zturner added a comment to D58513: [libFuzzer][Windows] Port fork mode to Windows.

Can you try mountvol <PATH>\mount C:\ and see what you print in this case?

This seems to break:

File: <PATH>\test-dir\dir\file-symlink2
File: <PATH>\test-dir\dir\y.txt
Dir: <PATH>\test-dir\dir
File: <PATH>\test-dir\dir-junction\y.txt
Dir: <PATH>\test-dir\dir-junction
File: <PATH>\test-dir\dir-Shortcut.lnk
File: <PATH>\test-dir\edge-case
File: <PATH>\test-dir\file.txt
FindFirstFileA failed with error 5 for directory: <PATH>\ss64\$Recycle.Bin\blah; exiting I'm not sure what the correct behavior in this case is though. Actually just use a different directory then. Like mount D: instead of C:. Recycle bin is protected, so you probably won't be able to get directly into it without special permissions. What I'm really wondering about is whether a volume mounted with mountvol will fall into your directory branch or your file branch. It should go into your directory branch. It takes the directory branch. Do we want it to take either though? Is this a common use case? This particular case has me worried because if LF used delete as its callback function for files it would be like calling rm -rf ~/ Thu, Feb 21, 2:39 PM · Restricted Project, Restricted Project zturner added a comment to D57896: Variable names rule. I can't argue with anything you say.. but I guess to reinforce your point introducing what is effectively a 3rd style would likely cause even more jarring... Zach isn't introducing a new style, the style already exists and is consistently used by what I think is our 3rd largest subproject. It happens not to be used at all by the two largest subprojects, but those subprojects already aren't consistent with themselves. I would not mind a more concerted effort to migrate to whatever style we pick, which was notably lacking last time around. Then the jarring inconsistencies would go away, and we could all get back to complaining about content and not style. Thu, Feb 21, 2:06 PM · Restricted Project, Restricted Project zturner added a comment to D58513: [libFuzzer][Windows] Port fork mode to Windows. Can you try mountvol <PATH>\mount C:\ and see what you print in this case? This seems to break: File: <PATH>\test-dir\dir\file-symlink2 File: <PATH>\test-dir\dir\y.txt Dir: <PATH>\test-dir\dir File: <PATH>\test-dir\dir-junction\file-symlink2 File: <PATH>\test-dir\dir-junction\y.txt Dir: <PATH>\test-dir\dir-junction File: <PATH>\test-dir\dir-Shortcut.lnk File: <PATH>\test-dir\dir-symlink\file-symlink2 File: <PATH>\test-dir\dir-symlink\y.txt Dir: <PATH>\test-dir\dir-symlink File: <PATH>\test-dir\edge-case File: <PATH>\test-dir\file-hardlink1 File: <PATH>\test-dir\file-hardlink2 File: <PATH>\test-dir\file-symlink File: <PATH>\test-dir\file.txt FindFirstFileA failed with error 5 for directory: <PATH>\ss64\$Recycle.Bin\blah; exiting

I'm not sure what the correct behavior in this case is though.

Actually just use a different directory then. Like mount D: instead of C:. Recycle bin is protected, so you probably won't be able to get directly into it without special permissions.

What I'm really wondering about is whether a volume mounted with mountvol will fall into your directory branch or your file branch. It should go into your directory branch.

That said, it looks like this does find a more general bug, which is that if the directory is inaccessible you will fail and terminate the iteration. You should probably handle this gracefully and just return from the function is FindFirstFile fails.

Thu, Feb 21, 2:03 PM · Restricted Project, Restricted Project
zturner added a comment to D58513: [libFuzzer][Windows] Port fork mode to Windows.

Can you try mountvol <PATH>\mount C:\ and see what you print in this case?

This seems to break:

File: <PATH>\test-dir\dir\file-symlink2
File: <PATH>\test-dir\dir\y.txt
Dir: <PATH>\test-dir\dir
File: <PATH>\test-dir\dir-junction\y.txt
Dir: <PATH>\test-dir\dir-junction
File: <PATH>\test-dir\dir-Shortcut.lnk
File: <PATH>\test-dir\edge-case
File: <PATH>\test-dir\file.txt
FindFirstFileA failed with error 5 for directory: <PATH>\ss64\\$Recycle.Bin\blah; exiting

I'm not sure what the correct behavior in this case is though.

Thu, Feb 21, 2:02 PM · Restricted Project, Restricted Project
zturner added inline comments to D58513: [libFuzzer][Windows] Port fork mode to Windows.
Thu, Feb 21, 1:52 PM · Restricted Project, Restricted Project
zturner added a comment to D58513: [libFuzzer][Windows] Port fork mode to Windows.

Can you try mountvol <PATH>\mount C:\ and see what you print in this case?

Thu, Feb 21, 1:49 PM · Restricted Project, Restricted Project
zturner added a comment to D57896: Variable names rule.

Since someone already accepted this, I suppose I should mark require changes to formalize my dissent

As it was Chris @lattner who accepted it, is your request for changes just based on the fact that it doesn't fit LLDB style?

(Side note, but I think everyones' opinions hold the same weight with regards to issues like this, and that is in part why changes like this are so difficult to move forward with. Because it takes a lot of consensus, not just one person, to drive a change.)

Thu, Feb 21, 1:12 PM · Restricted Project, Restricted Project
zturner added inline comments to D58513: [libFuzzer][Windows] Port fork mode to Windows.
Thu, Feb 21, 11:19 AM · Restricted Project, Restricted Project
zturner added inline comments to D58513: [libFuzzer][Windows] Port fork mode to Windows.
Thu, Feb 21, 11:14 AM · Restricted Project, Restricted Project
zturner added inline comments to D58513: [libFuzzer][Windows] Port fork mode to Windows.
Thu, Feb 21, 9:05 AM · Restricted Project, Restricted Project

Tue, Feb 19

zturner requested changes to D57896: Variable names rule.

Since someone already accepted this, I suppose I should mark require changes to formalize my dissent

Tue, Feb 19, 7:16 AM · Restricted Project, Restricted Project
zturner added a comment to D57896: Variable names rule.

Changed recommendation for acronyms from lower case to upper case, as suggested by several responses to the RFC.

I haven't been following the discussion closely - why is this the preferred direction? I don't think that things like "Basicblock *bb" or "MachineInstr *mi" will be confusing, and going towards a consistently leading lower case letter seems simple and preferable.

-Chris

Tue, Feb 19, 7:15 AM · Restricted Project, Restricted Project

Fri, Feb 15

Fri, Feb 15, 1:41 PM · Restricted Project
Fri, Feb 15, 1:07 PM · Restricted Project
zturner committed rG63c300cfc1f7: Don't include UnixSignals.h from Host. (authored by zturner).
Don't include UnixSignals.h from Host.
Don't include UnixSignals.h from Host.
Don't include UnixSignals.h from Host.
Fri, Feb 15, 12:47 PM · Restricted Project

Thu, Feb 14

Thu, Feb 14, 11:19 AM · Restricted Project
zturner added a comment to D58172: Embed swig version into lldb.py in a different way.

Long term, I wonder if we can just delete this entire modify-python-lldb.py script. it seems like the entire purpose is to make sure that the SB methods support iteration, equality, and some other basic stuff, and it does this by inserting lines of python code into the actual __init__.py file. It seems like we could just have the swig file say %pythoncode%{import sb_iteration}, then write a file called sb_iteration.py which we ship alongside the __init__.py, and have it dynamically add all of the things it needs at runtime. Then, we don't even need a a post processing step at all, which seems like a definite improvement.

Thu, Feb 14, 10:21 AM · Restricted Project

Fri, Feb 8

zturner added a comment to D57896: Variable names rule.

Is this actually any better? Whereas before we can’t differentiate type names and variable names, under this proposal we can’t differentiate type names and function names. So it seems a bit of “6 of 1, half dozen of another”

Fri, Feb 8, 8:59 PM · Restricted Project, Restricted Project
zturner added a comment to D57533: lit: support long paths on Windows.

I think getting the rest of unxutils is also a huge roadblock. So we could either add llvm-grep and all the other unix utils we rely on, or we could have a script checked in somewhere that downloads a known-good version of unxutils for use on Windows. That would fix this issue here and would make running llvm tests on Windows much easier in general.

Fri, Feb 8, 10:09 AM · Restricted Project

Thu, Feb 7

zturner updated the diff for D57911: [LLDB] Remove all abandoned LLDB bots.

I added the Fedora bot back and rebased this on top of Jan's patch from D54386.

zturner added a comment to D54386: Disable for new Linux OS runs: upload test traces.

I just uploaded a patch to delete a bunch of abandoned / broken LLDB bots, its' here. https://reviews.llvm.org/D57911

Thu, Feb 7, 10:37 AM · Restricted Project, Restricted Project
zturner added a comment to D57533: lit: support long paths on Windows.

I prefer if things "just work", so I'm not sure I understand where the pushback on this one is coming from. This isn't just an issue for buildbots, it's also an issue for developers who might have their build trees nested very deeply. Even on my local machine, I have some directories whose names are quite long because of the way I structure my build folder.

Thu, Feb 7, 10:36 AM · Restricted Project
Thu, Feb 7, 10:31 AM · Restricted Project

Wed, Feb 6

Can you rename these two tests to command-regex-delete.test and command-regex-unalias.test? Otherwise, lgtm

Wed, Feb 6, 1:42 PM · Restricted Project

Tue, Feb 5

zturner committed rGc5d68d499ab6: [PDB] Remove dots and normalize slashes with /PDBSOURCEPATH. (authored by zturner).
[PDB] Remove dots and normalize slashes with /PDBSOURCEPATH.
[PDB] Remove dots and normalize slashes with /PDBSOURCEPATH.
[PDB] Remove dots and normalize slashes with /PDBSOURCEPATH.
Tue, Feb 5, 4:50 PM · Restricted Project
zturner added inline comments to D57790: [CodeView] Fix cycles in debug info when merging Types with global hashes.
Tue, Feb 5, 4:15 PM · Restricted Project
zturner added inline comments to D57790: [CodeView] Fix cycles in debug info when merging Types with global hashes.
Tue, Feb 5, 4:05 PM · Restricted Project
Tue, Feb 5, 12:52 PM · Restricted Project

Ah, I see.

It feels a bit weird to use the native path functions only to then manually replace slashes. What do you think about:

sys::path::Style guessed_style = Config->PDBSourcePath.startswith("/")
? sys::path::Style::posix
: sys::path::Style::windows;

after the first return and then pass that to all the sys::path function calls in this function instead?

Tue, Feb 5, 12:41 PM · Restricted Project
// Only apply native and dot removal to the relative file path.  We want to
// leave the path the user specified untouched since we assume they specified
// it for a reason.

Is that necessary for anything? That doesn't look like the behavior we want -- we do want /pdbsourcepath:c:\foo\bar and a file with a relative path of ..\..\blah to become c:\blah. Can we undo that bit to fix (1)?

Tue, Feb 5, 11:58 AM · Restricted Project
Tue, Feb 5, 10:31 AM · Restricted Project

Mon, Feb 4

zturner added a comment to D56904: [NativePDB] Process virtual bases in the correct order.

Ping! What do you think about it?

Mon, Feb 4, 11:10 AM · Restricted Project

Fri, Feb 1

zturner added inline comments to D57552: Handle "." in target.source-map in PathMapListing::FindFiles.
Fri, Feb 1, 10:25 AM · Restricted Project, Restricted Project

Thu, Jan 31

zturner added a comment to D57533: lit: support long paths on Windows.

@ruiu, no unfortunately, not all the paths can be shortened in the swift test suite since it is such a heavy user of the clang modules, modules cache paths and module naming structure in clang is a problem. Perhaps if we could make %t in lit be short and clang module names to be short, it might be possible to get the paths to a size that works?

If you can do that, that's much more preferable solution than fixing the problem only at this location. I'd think if your test file get a very long pathname, this is not the only place that could break. For example, if you use rm on a test file and the test file gets a long pathname, that could fail.

Thu, Jan 31, 1:05 PM · Restricted Project

Wed, Jan 30

[RuntimeDyld] Don't try to allocate sections with align 0.
zturner updated the summary of D57482: [RuntimeDyld] Don't try to allocate sections with alignment 0..

I think it's possible to detect the case insensitivity of the file system itself, rather than relying on the operating system which as you point out is neither necessary nor sufficient to identify case insensitivity. But, also like you pointed out, that's another can of worms entirely, so I agree this is the simplest thing to make things better than before.

This is really cool, thanks for doing this!

Wed, Jan 30, 9:07 AM · Restricted Project, Restricted Project

Tue, Jan 29

Fix some warnings in building LLDB.
Fix some warnings in building LLDB.
zturner added inline comments to D57413: Fix some warnings with gcc on Linux.
zturner added inline comments to D57413: Fix some warnings with gcc on Linux.

Tue, Jan 29, 1:21 PM · Restricted Project

Mon, Jan 28

zturner added a comment to D57343: lit: Let lit.util.which() return a normcase()ed path.

Shouldn't the hash be case-insensitive on windows?

zturner added a comment to D56904: [NativePDB] Process virtual bases in the correct order.

Sorry, due to the way Phabricator re-orders feedback across files when sending the email notification, if you're reading my comments in email you have to read the previous email from the bottom up.

Mon, Jan 28, 10:43 AM · Restricted Project
zturner added a comment to D56904: [NativePDB] Process virtual bases in the correct order.

The above suggestion is admittedly minor, but since it's both a minor performance improvement and a minor readability/maintainability improvement, I think it's probably worth doing.

Mon, Jan 28, 10:39 AM · Restricted Project
zturner added a comment to D56126: [NativePDB] Add basic support of methods recostruction in AST.

Thanks for the reminder!

Mon, Jan 28, 10:22 AM · Restricted Project
Mon, Jan 28, 10:22 AM · Restricted Project
Mon, Jan 28, 9:53 AM · Restricted Project

Thu, Jan 24

[PDB] Increase TPI hash bucket count.
[PDB] Increase TPI hash bucket count.

Jan 22 2019

zturner added a comment to D56942: Change TPI Bucket size for PDBs from minimum to maximum.

When I apply this change locally, I get some test failures in LLD. Essentially, if you run llvm-pdbutil dump -types -type-extras foo.pdb we do some sort of crude hash verification. Basically, we take the hash that is in the PDB, then recompute what we think the hash should be, then compare the two, and if they're not equal we indicate that with a message. This message is trigger in the LLD tests, which makes me think that either something is wrong with the test, or something is wrong with the way we do the hash verification.

Jan 18 2019

Excellent find! Alexandre Ganea (cc'ed) reported to me offline that he had seen degraded watch-window performance when using clang-cl generated code, but we only had some guesses as to what was causing it (none of which was this). However, after reading your explanation, I'm pretty certain it had to be this. I don't think we would have discovered this without your patch, so thanks!

I think we will need to add this back at some point in the future, because it's necessary for incremental updates. But if you want to remove it in the meantime since there's no existing use, I suppose I can't object.

Jan 15 2019

zturner added a comment to D56741: [CMake] Explicitly list User32 as dependency of lldb-mi.

It seems like a pretty bad idea to have lldb-mi show a dialog box :-/

zturner added a comment to D56741: [CMake] Explicitly list User32 as dependency of lldb-mi.

Why do we need to link against user32? What kind of unresolved external are you getting?

zturner added inline comments to D56743: [llvm-rc] Support '--' for delimiting options from input paths.
In D56230#1358269, @Hui wrote:
• drop the if (m_entries[i].quote) branch. You don't need it here, and I don't believe it will be correct anyway, because all it will do is cause llvm::sys::flattenWindowsCommandLine to add one more quote level around that and escape the quotes added by yourself

The quote char might be specified through Args::AppendArgument at user's will.

Yes, but do you know what the user meant when he set it? I don't, but I'm pretty sure he did not mean it to be passed verbatim to the launched process. It certainly isn't what will happen on posix systems as there we just ignore it. Maybe nobody knows, which is why nobody sets it? I think it would be best to be consistent with posix systems, and ignore it here too (at least until we have a good reason to do otherwise).

I had to stop commenting because there's too many occurrences, but please remove auto pretty much throughout the patch. A reader of the code will not necessarily be aware or remember that the function returns an Optional, and they will read the code as "if the size is 0, report an error". So I think the type should be spelled explicitly in all cases.

Jan 14 2019

[SymbolFile] Remove SymbolContext parameter from FindTypes.
[SymbolFile] Remove SymbolContext parameter from FindTypes.
[SymbolFile] Remove the SymbolContext parameter from FindNamespace.
[SymbolFile] Remove the SymbolContext parameter from FindNamespace.
[SymbolFile] Rename ParseFunctionBlocks to ParseBlocksRecursive.
[SymbolFile] Rename ParseFunctionBlocks to ParseBlocksRecursive.

Jan 11 2019

In D56230#1354829, @Hui wrote:

I think the key problem here is to make sure the argument will be treated as a single argument to the process launcher.

To be specific to this case only, could we just provide a quote char to argument log file path and log channels on Windows?

The downside is one more #if is introduced.

#ifdef _WIN32
char quote_char= '"';
#else
char quote_char='\0';
#endif

 std::string env_debugserver_log_channels =
host_env.lookup("LLDB_SERVER_LOG_CHANNELS");
if (!env_debugserver_log_channels.empty()) {
debugserver_args.AppendArgument(
llvm::formatv("--log-channels={0}", env_debugserver_log_channels)
.str(), quote_char);
}
zturner added a comment to D47073: Document and Enforce new Host Compiler Policy.

I'd like to see docs/GettingStarted.rst updated to include some language from what Chandler mentioned. In particular upgrading a toolchain has to be *motivated* by explicit benefits, and the toolchains dropped must be evaluated with respect to the general availability for the users.

Fix build breaks after the ParseCompileUnit changes.
Fix build breaks after the ParseCompileUnit changes.
[SymbolFile] Make ParseCompileUnitXXX accept a CompileUnit&.
[SymbolFile] Make ParseCompileUnitXXX accept a CompileUnit&.

Jan 10 2019

zturner added a comment to D56564: [SymbolFile] Make ParseCompileUnitXXX accept a CompileUnit&..

All the change to the symbol vendor make sense, but it seems like all of the call sites should be:

cu->GetLanguage();
cu->ParseFunctions();
cu->GetLineTable();
cu->ParseDebugMacros();
cu->GetSupportFiles();
cu->ParseTypes();

Some of these calls might already be there, but is seems like we should initiate these calls from the CompileUnit class.

zturner retitled D56564: [SymbolFile] Make ParseCompileUnitXXX accept a CompileUnit&. from [SymbolFile] to [SymbolFile] Make ParseCompileUnitXXX accept a CompileUnit&..