- User Since
- Sep 23 2014, 10:11 AM (351 w, 6 d)
Fri, Jun 18
Just a few nits!
Fixed a case where the accessor was being used twice in the same function and fixed a typo in a comment.
Yes, good to go!
Lets try the diamond character for the boolean stuff unless anyone has any objections. Maybe handle a few more keys for the boolean field as suggested in the comments. This will be good to go after these changes!
Just one issue regarding the stop ID for post mortem. Once this is resolved, we will be good to go!
Thu, Jun 17
Wed, Jun 16
a few nits!
Looks very nice.
Fri, Jun 11
Accepting since no one else had comments. Sorry for the delay.
I mostly commented on ThreadTrace.h and we should resolve the questions in there before reviewing the rest of this patch.
Many minor things, but looking good! I look forward to seeing this get checked in
Thu, Jun 10
Once teemperor's issues are resolved this looks good to me!
Mon, Jun 7
One quick either documentation update for the error string, or switch to ConstString... Probably best to the the error string docs as if we use fancy C++ union stuff it might not compile for everyone on all C++ compilers..
So it would be nice to try and not encode errors into the TraceInstruction class and deal with any errors at decode time.
Thu, Jun 3
Wed, Jun 2
If no one else comments in the next 2 days, I will accept this patch. So speak up soon if you have an opinion.
Tue, Jun 1
Fix the test variable names and this is good to go
Thu, May 27
Wed, May 26
I am ok with all of the DWARF unwind changes. Someone else should give the ok for the MC stuff.
Mon, May 24
Do we want any safeguard on StoredDeclsList::prependDeclNoReplace(...)? Right now it can still cause duplicate NamedDecl pointers. I fix that tested helps prevent this:
May 21 2021
The only last nit is we are passing the report progress callback around all over the place. I think it would be nicer to just make a static function that we can call and remove the passing of the instance variable for the report callback all over to the event classes.
May 20 2021
I would be ok with this. Can you track down the original author and add them to the review of this to be sure they aren't using it anymore, and if they need tracing that they have everything they need with your new functionality?
Contents looks good, just need a test to ensure that warnings are quieted. There should be a test for each kind of warning, so it should be easy to run that same command with the --quiet option and verify that the warning does _not_ appear.
May 19 2021
Looks good to me. Thanks for the changes.
May 18 2021
May 17 2021
I would go ahead and request commit access, but at least there is the Phabricator link in the commit that shows that you authored the diff. This is the main reason you want to get commit access, so you everyone can easily see who did the patches.
You should probably get Phabricator working: https://llvm.org/docs/Phabricator.html
No objections from me. Anyone else?
May 15 2021
May 14 2021
Was this all fixed by the auto linter?
I had run statistics on a few hundred DWARF files way back when and came up with the original 14-20 byte number, but this was a long long time ago (at least over 10 years). With newer DWARF versions, and with optimizations this can easily change. So it would be great to know what the minimum value should be set to by default. I agree that adding a statistic would be nice so we can track this. I will test some recent DWARF files out and see what my numbers show and report back in this patch.
May 12 2021
May 11 2021
There are unit tests that could be added if you would like to test a bad case, but existing unit tests should still pass with this.
On optimization idea is that is one input file is specified, we could specify only addresses in the STDIN? Something like:
So looks like we just need tests:
- check error when user specifies --addresses-from-stdin and also an address
- check error when user specifies --addresses-from-stdin and also an input file
- check for successful lookups on multiple address + file tuples
And if would be good to specify the input format for the --addresses-from-stdin option in the option description.
Sounds like, after I reread the description, that we have other tools in the llvm ecosystem that use this <addr> <file> format... Sorry for the noise. I will add inline comments to clarify any needed changes.
May 10 2021
Thanks for combining the functions and fine not to use Optional is we have a good value to indicate no timeout.
May 7 2021
May 5 2021
LGTM! Just had one nit code edit suggestion.
May 4 2021
Just fix the one issue where we use the FileSpec operator== and this is good to go!
May 3 2021
Apr 28 2021
If other architectures are emitting this language for assembly files, then this LGTM.
Apr 26 2021
A bit of history on laying out classes: layout used to be a problem before we were able to give all of the field offsets directly to clang to assist in laying out. The main issues were:
- #pragma pack directives are not in DWARF so we must use the DW_AT_data_member_location
- Some DWARF optimizations cause class definitions to be omitted and we can have a class A that inherits from class B but we have no real definition for class B, just a forward declaration. In this case we will create a class A that inherits from a class B that has nothing in it, but the field offsets will ensure we show all other instance variables of class A correctly. Furthermore, we have code that can detect when we have such a case and it can grab the right definition for class B when it is imported into another AST, such as when evaluating an expression. This will only happen if class B is in another shared library from class A, and if we do have debug info for class B.
- any other keywords or attributes such as [[no_unique_address]] that can affect layout that don't appear in DWARF.
This will be tricky to do right and I really don't know how much extra performance we will get out of this for all of the possible issues we will can introduce by multi-threading things. That being said, I am happy to help see if this will actually improve performance.
Apr 23 2021
Apr 22 2021
I would be fine with DWARFDie getting bigger to 24B. These objects are used temporarily and not used as part of the storage of all of a DWARFUnit's DIEs. DWARFUnit objects store an array of DWARFDebugInfoEntry objects and those are what we care about not getting bigger.
Apr 21 2021
Much of this patch requires all places that took a DWARFDie object now also must take an extra parameter. Is there really no way to have DWARFDie be able to discover the main unit via its contained DWARFUnit? DWARFDie objects are transient, so they can contain more information that just what they currently contain:
Apr 20 2021
The title should be updated on this diff as well. Set the "json" right now