- User Since
- Jun 4 2013, 6:02 AM (468 w, 4 d)
Tue, May 10
Mon, May 9
I'm afraid I had to revert this, as it was causing hangs in TestMultipleDebuggers.py.
Sure, that's great.
Wed, May 4
Heh, well.. there definitely are, though a lot of things that come to mind right now are not really suitable for a first-time contributor. Is there any specific are that you would like to know (or that you know already)?
Looks good. Thanks for the patch. Do you need me to commit this for you?
Yeah, I guess that what I am saying.
Awesome. Thanks for your patience.
Just my opinion, but I wouldn't like to complicate the logging architecture with file-only log statements and cross-channel dependencies. If you want to cross-reference some other logging channel with the executed commands then I'd say you should enable both channels. We can definitely talk about making the command feature more prominently in the log stream.
Tue, May 3
Mon, May 2
There were probably easier ways to test this, but it's nice to have the core file nonetheless. LGTM, assuming the core is of a reasonable size.
Fri, Apr 29
Apr 28 2022
Sure, but is the version control system the right tool to do that? I think it'd be better to have the test itself confirm the consistency of the data (or convert it into the right format at runtime) and not rely on magical conversions within the VC tool.
I believe (based on some similar issues I was solving in lldb) that -text instead of binary is sufficient to prevent git from fidlling with the contents, while maintaining the ability to diff the files.
Here's the updated version. I'll place my new findings into the patch
One could add a unit test for the CFD overload, though, as this appears to be the only callsite (or other callsites were equally untested and broken), we could also remove it altogether.
Yeah, this was very surprising.
Apr 27 2022
For future reference, it's better to generate a full diff when uploading a patch manually. Here are some ways to do that.
FWIW, I don't think that this feature, as it is implemented right now, has any chance of working on windows. Windows does not have the equivalent of .symtab (only .dynsym), so we cannot use that as a poor man's debug info substitute -- the debug info is either there -- or it isn't.
Apr 26 2022
Apr 25 2022
Apr 22 2022
Thanks, for your patience. Overall, I'd say this looks pretty clean now -- much cleaner than I originally thought it could be. I don't have any further comments here.
Yes, that's pretty much what I had in mind. Thanks.
The patch description could definitely use more details about the motivation for the change, and a description of how it works. Apart from the fact that it uses the raSearch feature I introduced a while back, I don't know much about how it works (and given the planned changes, I am not going to try to understand that), but I think I can provide some details/thoughts about the motivation.
Apr 21 2022
remove the stray binary
- address review comments
Apr 20 2022
Yeah, I suppose we can do that. If this turns out to be more widespread, then I'd rather disable the tidy check (or perhaps the clang warning) for the unittest files instead.
Apr 14 2022
Yes, I think that's exactly what I had in mind. Basically, the idea is to structure things such that the ondemand class can sit between the actual symbol file class and the outside world, but that it does not (and cannot) interfere with any of the interactions that happen inside a symbol file class. If it works, I think that kind of a setup would be much cleaner/understandable. As it stands now, one has to consider the possibility that any action inside the real symbol file can reenter the ondemand instance, which makes it harder to reason about.
Apr 13 2022
Apr 12 2022
This is great. Thanks for doing that.
I am trying to get rid of the friendship declaration and the forwarding of protected methods. I have no issue with making GetCompileUnitAtIndex virtual -- that is pretty much a necessity for this feature (*). However, SetCompileUnitAtIndex seems like an internal implementation detail of a SymbolFile instance (that's why it's protected), and it would be better if the new class didn't have to touch it. I wouldn't call the new class the "real" symbol file. I don't see it as any more real as any other subclass. The way I'd look at it is that the new class just contains some helpful implementation that can be useful to a specific symbol file class. An individual symbol file class may choose to use it, but it could also implement the pure SymbolFile interface is a completely different way, if it knows how.
Apr 11 2022
I'm looking at the SymbolFileOnDemand friendship and the forwarding of protected methods (mostly dealing with compile unit lists). Does this by any chance have something to do with the fact that there are now two compile unit lists (one in the actual symbol file, and one in the wrapping ondemand class? Would it be possible to avoid that by making SymbolFile a stateless interface (just methods, no member variables), and having a putting the member variables and other utility functions into a separate class that the real symbol files would inherit from? Then only SymbolFileOnDemand would inherit from SymbolFile directly, and it would only add the state that it needs (the bool flag, and the actual symbol file pointer).
It does say "patches that meet likely-community-consensus requirements can be committed prior to an explicit review" and "where there is any uncertainty, a patch should be reviewed prior to being committed".
It can be hard to judge what is a likely-community-consensus without being an active member of the community, which is why it's safer to go down the pre-commit review path.
Apr 8 2022
I think this is actually a wider problem. I also ran into this recently (with triples), but didn't have time to create a patch yet. I don't think that None as the "actual" value should ever be considered a match (except when the pattern is None, I guess), regardless of whether it is a debug info variant, triple, or anything else. (None as a "pattern" should of course continue to match anything.)
The reason for the funny /*override*/ thingy in the mock classes is that it is impossible (in the gmock version that we use) to add the override keyword to the methods overridden by the MOCK macros. Then, having override keywords on hand-written methods triggered -Winconsistent-missing-override.
Apr 7 2022
Apr 6 2022
Yes, that's the way we usually deal with this.
The gcc behavior is quite annoying here...
This is great. I've been bothered by this for quite a while, but I didn't realize the fix is that easy.
Apr 5 2022
Looks good. Thanks for your patience.
Note that this patch could have been shorter if I simply changed LoadModuleAtAddress() to always pass false for notify (all callers currently call ModulesDidLoad() afterwards) and added a note to LoadModuleAtAddress() docs about it, but I don't know if that's a good design choice.
Reopening, since the last attempt was quite some time ago, and the rebase was