- User Since
- May 26 2014, 12:49 PM (195 w, 2 d)
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.
Fri, Feb 16
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?
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.
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.
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?
Anyone have any comments on this?
Thu, Feb 15
Wed, Feb 14
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.
Tue, Feb 13
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?
Aaron, do you remember why you added a check for width == 0 here? Would it ever not be 0?
Mon, Feb 12
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.
Fri, Feb 9
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.
Thu, Feb 8
Sorry for the delay on this one, looks good.
Looks good. Wish this function took a StringRef so you could just say return name.startswith("?") || name.startswith("_Z")
(I can review too btw, just would suggest adding him also)
+rnk, in the future add him on anything MSABI specific.
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.
Modified the tests as suggested by @aprantl
Adds tests for bitcode upgrade and assembly roundtripping (as well as the necessary fixes to make these tests pass).
Wed, Feb 7
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
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.
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.
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.
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.
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.
Tue, Feb 6
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.
Mon, Feb 5
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.
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?
Very excited to see someone doing this. We've been meaning to do it for some time but always get sidetracked on other things.
Yea this seems like a good candidate for an lldb-test test. Something like this.
Fri, Feb 2
This was merged into D42762
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.
- 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.
- 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.
- Changed "clang" to "llvm"
- Merged D42769 into this patch.
- 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.
Thu, Feb 1
Wed, Jan 31
This update brings a couple of minor tweaks.
I made a few minor changes in this update.
Tue, Jan 30
Mon, Jan 29
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.
If we just need to test completion, write a lit-style test that uses lldb-test that looks like this:
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.
Fri, Jan 26
Return an empty record when remapping fails.
Thu, Jan 25
Tue, Jan 23
looks good. May as well fix the BCD typo while you're here though (either here or in a followup patch)
Jan 21 2018
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);
Trivial fixes like this don't need to be reviewed, you can just commit them. Thanks for your work btw!
Jan 20 2018
Jan 19 2018
Sorry for forgetting about this! lgtm
Jan 18 2018
Jan 17 2018
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 16 2018
LGTM, sorry for the delay, thanks for reminding me.