- User Since
- May 26 2014, 12:49 PM (182 w, 3 d)
Ahh, sorry about that. One of the Phabricator triggers must have added me automatically.
I absolutely do not want to disable support for ConcRT under any circumstances unless we find that it is a completely broken library, which I highly doubt. Even then, I would want to ask Microsoft to fix it.
Tue, Nov 21
Out of curiosity, what do we use LockFileManager for in LLVM, and what are you trying to use it for in this specific use case? Because of certain quirks in Windows file system semantics, lock files are in general not a robust solution most of the time -- maybe even ever.
Mon, Nov 20
Fri, Nov 17
looks good to me, rnk@ are you ok?
There's been some talk about making LLVM_OPTIMIZED_TABLEGEN the default, but it hasn't happened for whatever reason.
Thu, Nov 16
Wed, Nov 15
I'm not suuuper opposed, but at the same time if this code is bothering people (and it is, consistently), I don't changing the requirements from "confusing rule A" to "confusing rule B" is going to solve the long term burden that people keep running into.
On second thought I actually think the strategy used here is fine. But we have llvm::WritableBinaryStream which could server as a single base interface for a mapped_file_region based implementation as well as a malloc-based implementation.
Mon, Nov 13
Fri, Nov 10
Seems like a good idea to me, for consistency.
I'd be open to having another organizational component that isn't Utility. But as Jim said, there isn't a critical mass of stuff yet in Utility to figure out where it makes sense to draw the line.
Thu, Nov 9
yay, huge space gains ftw.
Ahh sorry, I may have misunderstood your previous question. I thought we were talking about setting core.autocrlf=false at the global LLVM .gitattributes file, and then not having anything special for this one particular test.
Updated with the suggestions. The -imsvc argument is nice, and allows us to get rid of manually specifying the resource directory, which was one of the FIXMEs I had before. So that is now fixed.
Wed, Nov 8
Tue, Nov 7
I went ahead and applied the same optimization for llvm::Expected, and also incorporated other suggestions. Even though Expected wasn't showing up on the profile, this only arose because we happened to be a heavy user of Error. If someone else happened to be a heavy user of Expected, we'd see the same issues arise.
Mon, Nov 6
Fri, Nov 3
Definitely. I'm in no rush. I'll push out a message to llvm-dev before the day's over.
Thu, Nov 2
Update: I actually do see where you're testing that they work, but I'm not convinced that the tests are sufficient. For several reasons.
First off, let me say this is very cool. I look forward to the day when all commands are builtin, and we can switch every client to the builtin lit shell.
Looks good, mostly just nitpicks. Can you also add a test for this? We've checked in an several actual PDBs into test/tools/llvm-pdbutil/Inputs, but you'd need to add a command line option to the pretty mode of llvm-pdbutil so that it can dump it. Perhaps making it a hidden option. Hopefully the PDBs that are checked in already will cause DIA to return something interesting for the tables.
Ok, I wasn't aware of the libedit problem.
Agreed. Optional should basically be called OneOrZero. And if you think about it in those terms, it obviously doesn't make sense to allow it to be specified more than once.
Wed, Nov 1
I actually think that the llvm-64-bits feature should go in the common config.py setup, but I don't think we're quite there yet. It would require some additional work to make sure everyone who uses LLVM can agree on a common set of variables that must be passed in through configure, and currently we don't have that.
Nice! I'm looking forward to reviewing this tomorrow.
Tue, Oct 31
Mon, Oct 30
Hmm, weird. Maybe it already is? Even though I didn't see it on the original email. Anyway, ignore my last suggestion.
Would you mind deleting and re-creating this revision with lldb-commits added as a subscriber? I don't think it's sufficient to just "add" it as a subscriber after the fact, I think it has to be done as part of the initial creation, or for some reason it won't show up on the mailing list.
Fri, Oct 27
Thu, Oct 26
So when you're using ds2 as the remote, are you still using the normal lldb test suite? dotest.py? If so, then we could have a test decorator that says @expectedFailure(not(debugserver=ds2)) or something. Then, even though you're the only one that can run it, at least YOU are sure it works. Some coverage is better than nothing. Is something like this possible?
Ahh, you might also run the exact same test again but where you've loaded this inside of main with LoadLibrary instead of relying on early binding by the loader.
A test would be something like this:
Do I understand correctly that this will insert breakpoints on *all* clang diagnostics? That's not necessarily bad, but I was under the impression originally that it would let you pick the diagnostics you wanted to insert breakpoints on.
Wed, Oct 25
We really should be using llvm_config.get_process_output() here, fwiw, but as you said, this patch is just fixing an execution failure.
Oct 24 2017
Let me know if this is correct. I think we can actually get (potentially very large) savings from this.
Adding more Windows people, because as I look over this code, it actually looks wrong to me. The value we pass for MaximumFileSize seems unnecessarily large. I'm going through the docs at the moment to figure out the correct thing to do here.
I will ping them for some numbers and more details of their test setup. Regardless, I didn't mean to derail the code review. But, I really really want to reach a point where we can stop falling back on the "we need to be safe even in the presence of stuff that is clearly not user input" argument. I understand the concerns, but I don't think this is a reasonable path forward for the project. If it's not user input, if we own it, then we can make whatever assumption we want that leads to the best performance and memory usage characteristics.
I know you're doing things the way it's always been done, but I want to start questioning some long-standing practices :) So I'm not picking on you specifically, but anywhere we can migrate towards something better incrementally, I think we should do so.
Oct 23 2017
Ok the issue is that you cant use CMake generator expressions in this way. This should work though: