Page MenuHomePhabricator

shafik (Shafik Yaghmour)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 12 2018, 2:31 PM (89 w, 5 d)

Recent Activity

Today

shafik added inline comments to D77153: [lldb/DataFormatters] Display null C++ pointers as nullptr.
Tue, Mar 31, 12:33 PM · Restricted Project

Yesterday

shafik added reviewers for D76964: Fix an issue where the IgnoreName function was not allowing "Class" to be looked up inside a namespace or other class.: shafik, teemperor.
Mon, Mar 30, 10:49 AM · Restricted Project
shafik added a comment to D76964: Fix an issue where the IgnoreName function was not allowing "Class" to be looked up inside a namespace or other class..

You also mentioned the id case failing as well. We should add that case to the test as well.

Mon, Mar 30, 10:49 AM · Restricted Project

Fri, Mar 27

shafik committed rG8016d61e3cf4: [LLDB] CPlusPlusNameParser does not handles templated operator< properly (authored by shafik).
[LLDB] CPlusPlusNameParser does not handles templated operator< properly
Fri, Mar 27, 2:54 PM
shafik closed D76168: CPlusPlusNameParser does not handles templated operator< properly.
Fri, Mar 27, 2:54 PM · Restricted Project
shafik committed rG00c8120acbac: [LLDB] Fix handling of bit-fields when there is a base class when parsing DWARF (authored by shafik).
[LLDB] Fix handling of bit-fields when there is a base class when parsing DWARF
Fri, Mar 27, 11:30 AM
shafik closed D76808: Fix handling of bit-fields when there is a base class when parsing DWARF.
Fri, Mar 27, 11:30 AM · Restricted Project
shafik updated the diff for D76808: Fix handling of bit-fields when there is a base class when parsing DWARF.

Minor fixes

Fri, Mar 27, 9:41 AM · Restricted Project
shafik updated the diff for D76168: CPlusPlusNameParser does not handles templated operator< properly.

Fixing up loose language in the comments and adding periods.

Fri, Mar 27, 9:41 AM · Restricted Project

Thu, Mar 26

shafik updated the diff for D76808: Fix handling of bit-fields when there is a base class when parsing DWARF.
  • Expanded comments
  • Adjusted conditionals
Thu, Mar 26, 3:14 PM · Restricted Project
shafik added inline comments to D76808: Fix handling of bit-fields when there is a base class when parsing DWARF.
Thu, Mar 26, 3:14 PM · Restricted Project
shafik updated the diff for D76168: CPlusPlusNameParser does not handles templated operator< properly.

Addressing comments:

  • Adding more detailed comments
  • Adding test for cases that currently fail b/c we don't enable C++20
Thu, Mar 26, 12:29 PM · Restricted Project
shafik accepted D76840: [lldb] Fix another crash in covariant type handling.
Thu, Mar 26, 10:18 AM · Restricted Project

Wed, Mar 25

shafik added reviewers for D76805: Fix SourceManager::SourceFileCache insertion: labath, JDevlieghere.
Wed, Mar 25, 3:44 PM · Restricted Project
shafik added reviewers for D76804: Add new LLDB setting: use-source-cache: labath, JDevlieghere.
Wed, Mar 25, 3:44 PM · Restricted Project
shafik created D76808: Fix handling of bit-fields when there is a base class when parsing DWARF.
Wed, Mar 25, 3:44 PM · Restricted Project
shafik added a reviewer for D76808: Fix handling of bit-fields when there is a base class when parsing DWARF: vsk.
Wed, Mar 25, 3:44 PM · Restricted Project

Tue, Mar 24

shafik added a reviewer for D76736: [LLDB] Fix parsing of IPv6 host:port inside brackets: labath.

We need tests it looks like UriParserTest.cpp is the right place.

Tue, Mar 24, 7:09 PM · Restricted Project

Mon, Mar 23

shafik committed rGa567d6809e15: [DataFormatters] Add formatter for libc++ std::unique_ptr (authored by shafik).
[DataFormatters] Add formatter for libc++ std::unique_ptr
Mon, Mar 23, 12:01 PM
shafik closed D76476: Add formatter for libc++ std::unique_ptr .
Mon, Mar 23, 12:01 PM · Restricted Project

Fri, Mar 20

shafik updated the diff for D76476: Add formatter for libc++ std::unique_ptr .

Adding addition tests for references

Fri, Mar 20, 3:12 PM · Restricted Project
shafik updated the diff for D76476: Add formatter for libc++ std::unique_ptr .

Addressing comments on naming and not duplicating the regex

Fri, Mar 20, 11:23 AM · Restricted Project
shafik added inline comments to D76476: Add formatter for libc++ std::unique_ptr .
Fri, Mar 20, 11:23 AM · Restricted Project
shafik added inline comments to D76471: Remap the target SDK directory to the host SDK directory.
Fri, Mar 20, 9:44 AM

Thu, Mar 19

shafik created D76476: Add formatter for libc++ std::unique_ptr .
Thu, Mar 19, 9:22 PM · Restricted Project
shafik added inline comments to D76385: Allow remapping Clang module include paths.
Thu, Mar 19, 2:14 PM · Restricted Project

Wed, Mar 18

shafik added a comment to D76168: CPlusPlusNameParser does not handles templated operator< properly.

Long-term I would like to modify clang to stop doing that for LLDB, but LLDB will still have to support older compilers for a while. So I think this fix is still needed.

So is this some alternative to D75761 (where we'd use CPlusPlusNameParser to decode DW_AT_names of templates)? If so, I think that is an interesting direction, but beware that that class is kind of meant for processing the demangler output. The contents of DW_AT_name looks a bit like a demangled name, but in reality there are some deviations from that format (which is why I did not recommend this direction initially -- but I am not against it either).

Also it looks like llvm and gnu demanglers disagree on the exact formatting of demangled operator names:

$ c++filt _ZlsI1AEvT_S1_
void operator<< <A>(A, A)
$ llvm-cxxfilt _ZlsI1AEvT_S1_
void operator<<<A>(A, A)

I think the gnu format is superior (and unambiguous) so we could change llvm to match that -- and this change probably won't require any kind of compatibility hacks.

Wed, Mar 18, 1:35 PM · Restricted Project

Tue, Mar 17

shafik committed rG9e2715aaacaa: [lldb] Remove template parameters from FunctionTemplateDecl names (authored by shafik).
[lldb] Remove template parameters from FunctionTemplateDecl names
Tue, Mar 17, 11:18 AM
shafik closed D75761: [lldb] Remove template parameters from FunctionTemplateDecl names.
Tue, Mar 17, 11:17 AM · Restricted Project

Mon, Mar 16

shafik updated the diff for D75761: [lldb] Remove template parameters from FunctionTemplateDecl names.

Addressing minor comments

Mon, Mar 16, 4:24 PM · Restricted Project
shafik retitled D75761: [lldb] Remove template parameters from FunctionTemplateDecl names from [lldb] Fix to get the AST we generate for function templates to be closer to what clang generates and expects to [lldb] Remove template parameters from FunctionTemplateDecl names.
Mon, Mar 16, 3:51 PM · Restricted Project
shafik added a comment to D76168: CPlusPlusNameParser does not handles templated operator< properly.

I am not sure what the usage scenario is that this is meant to support. Is it user input that tries to name a specialization of a template operator< without separation to prevent tokenization as operator<<? I think that case should qualify as user error.

Both C++17 subclause 17.2 [temp.names] and N4849 subclause 13.3 [temp.names] imbue the "angle-bracket" treatment of < to after identification of a name (not to the formation of the name itself) and does involve special treatment of <<. Special treatment in tokenization (C++17 subclause 5.4 [lex.pptoken]; N4849 subclause 5.4 [lex.pptoken]) for angle brackets extends to <: and not to <<.

All of Clang, GCC, ICC, and MSVC do not compile the following:

enum E { E0 };
template <typename T> void operator<(E, T);

void g() {
  operator<<int>(E0, 0);  // does not compile
}

Compiler Explorer (godbolt.org) >>link<<.

In other words, where is the input coming from? Should the producer of the input be corrected instead?

Mon, Mar 16, 12:34 PM · Restricted Project
shafik added inline comments to D76163: [lldb/Reproducers] Decode run-length encoding in GDB replay server..
Mon, Mar 16, 10:52 AM · Restricted Project

Fri, Mar 13

shafik updated the diff for D75761: [lldb] Remove template parameters from FunctionTemplateDecl names.

Addressing comments

Fri, Mar 13, 5:17 PM · Restricted Project
shafik created D76168: CPlusPlusNameParser does not handles templated operator< properly.
Fri, Mar 13, 5:17 PM · Restricted Project
shafik added a comment to D76168: CPlusPlusNameParser does not handles templated operator< properly.

My fix in ConsumeOperator() is not proper but if everyone feels this is correct approach I will create member functions to deal with this cleanly.

Fri, Mar 13, 5:17 PM · Restricted Project
shafik added reviewers for D76108: do not merge - [clang] fix EndLoc of some Statements: aaron.ballman, rsmith, ymandel, rjmccall.

Adding potentially relevant reviews.

Fri, Mar 13, 12:23 PM · Restricted Project

Thu, Mar 12

shafik abandoned D76080: Adjust error_msg handling for expect_expr in lldbtest.py.

After discussing this @teemperor in some detail it looks like getting the error_msg case to work well is not a trivial task. So for these cases we should revert to just using expect. I think the plan is that he will remove the feature for now.

Thu, Mar 12, 7:04 PM
shafik updated the diff for D76080: Adjust error_msg handling for expect_expr in lldbtest.py.

Moving to using assertIn as suggest in comment.

Thu, Mar 12, 4:18 PM
shafik added a comment to D76080: Adjust error_msg handling for expect_expr in lldbtest.py.
  • All of the asserts should print a useful error when failing (i.e., one that allows us to directly write a fix). You could do assertIn which is clearer than find(...) and automatically gives an error message that is useful. For the assertTrue just print the unexpected result in the assert message as the old code did.
Thu, Mar 12, 4:18 PM
shafik updated the diff for D76080: Adjust error_msg handling for expect_expr in lldbtest.py.

Incorporate feedback on how to verify the results.

Thu, Mar 12, 12:28 PM
shafik added a comment to D76080: Adjust error_msg handling for expect_expr in lldbtest.py.
Thu, Mar 12, 12:28 PM
shafik created D76080: Adjust error_msg handling for expect_expr in lldbtest.py.
Thu, Mar 12, 10:51 AM
shafik added a comment to D76080: Adjust error_msg handling for expect_expr in lldbtest.py.

I am open to suggestions on alternative approaches, for some context I ran into this trying to add a failing test to D75761 as was suggested.

Thu, Mar 12, 10:51 AM

Wed, Mar 11

shafik added a comment to D75562: Add an opque payload field to lldb::Type (NFC)..

I want to see how you end up resolving the comments on payload being a plain integer type in D75626 before looking at this again. Maybe it makes more sense to use a type, the use is pretty clever but perhaps makes for opaque code in some places.

Wed, Mar 11, 10:45 AM · Restricted Project
shafik accepted D75560: Make Decl:: setOwningModuleID() public. (NFC).

LGTM

Wed, Mar 11, 10:45 AM · Restricted Project

Tue, Mar 10

shafik added inline comments to D75626: Add support for owning module information to TypeSystemClang..
Tue, Mar 10, 11:00 PM
shafik added inline comments to D75626: Add support for owning module information to TypeSystemClang..
Tue, Mar 10, 4:58 PM
shafik updated subscribers of D75922: [ASTImporter] Compare the DC of the underlying type of a FriendDecl.

I believe that your main example violates basic.scope.class p2:

Tue, Mar 10, 4:26 PM · Restricted Project
shafik added a comment to D75715: Switch TypeSystemClang over to CreateDeserialized() (NFC).

A bunch of small comments but a few more serious ones as well.

Tue, Mar 10, 3:21 PM · Restricted Project, Restricted Project
shafik added inline comments to D75715: Switch TypeSystemClang over to CreateDeserialized() (NFC).
Tue, Mar 10, 2:46 PM · Restricted Project, Restricted Project
shafik updated the diff for D75761: [lldb] Remove template parameters from FunctionTemplateDecl names.

Move to using expect_expr in the test.

Tue, Mar 10, 10:19 AM · Restricted Project

Mon, Mar 9

shafik added a comment to D75761: [lldb] Remove template parameters from FunctionTemplateDecl names.

It's a pity that the clang's DW_AT_name value is so ambiguous. For example, gcc would output the name in the commit message as operator< <A::B>, which is a lot more parsable (for computers and people). Have you looked at changing clang's output to make it more like gcc's ? I know it would only help new compilers, but maybe for such a special case, this does not matter?

Or, even if we do end up adding some compat parsing code, that would reduce the need for it to be super exact. I'm pretty sure that the current code does not handle all cases correctly. E.g., I believe it will break on something like operator<<<&operator>> > (mangled name _ZlsIXadL_Zrs1AS0_EEEvS0_S0_, godbolt link).

As an alternative, we could try extracting the same information from the mangled name (via llvm::ItaniumPartialDemangler::getFunctionBaseName), at least when DW_AT_linkage_name is present (not all compilers emit that attribute but I am not sure if anyone has tested if lldb actually works without it).

Mon, Mar 9, 5:17 PM · Restricted Project
shafik updated the diff for D75761: [lldb] Remove template parameters from FunctionTemplateDecl names.

Moving to using ItaniumPartialDemangler for now.

Mon, Mar 9, 5:17 PM · Restricted Project
shafik added a comment to D75740: [ASTImporter] Corrected import of repeated friend declarations..

LGTM WDYT @teemperor

Mon, Mar 9, 12:25 PM · Restricted Project
shafik accepted D71018: [ASTImporter] Improved import of TypeSourceInfo (TypeLoc).

LGTM

Mon, Mar 9, 10:46 AM · Restricted Project

Fri, Mar 6

shafik created D75761: [lldb] Remove template parameters from FunctionTemplateDecl names.
Fri, Mar 6, 11:35 AM · Restricted Project

Wed, Mar 4

shafik committed rG37549464c13e: [dsymutil] Fix template stripping in getDIENames(...) to account for overloaded… (authored by shafik).
[dsymutil] Fix template stripping in getDIENames(...) to account for overloaded…
Wed, Mar 4, 3:15 PM
shafik closed D75545: [dsymutil] Fix template stripping in getDIENames(...) to account for overloaded operators.
Wed, Mar 4, 3:14 PM · Restricted Project
shafik updated the diff for D75545: [dsymutil] Fix template stripping in getDIENames(...) to account for overloaded operators.

Addressing comment

  • Converting function to static Vs anon namespace
  • Updating naming
  • moving assignment into if
  • clang-format
Wed, Mar 4, 10:39 AM · Restricted Project
shafik updated the diff for D75545: [dsymutil] Fix template stripping in getDIENames(...) to account for overloaded operators.

Factored out template parameter stripping into its own function.

Wed, Mar 4, 10:05 AM · Restricted Project

Tue, Mar 3

shafik added inline comments to D75545: [dsymutil] Fix template stripping in getDIENames(...) to account for overloaded operators.
Tue, Mar 3, 5:16 PM · Restricted Project
shafik added inline comments to D75545: [dsymutil] Fix template stripping in getDIENames(...) to account for overloaded operators.
Tue, Mar 3, 5:16 PM · Restricted Project
shafik updated the diff for D75545: [dsymutil] Fix template stripping in getDIENames(...) to account for overloaded operators.
  • Addressing comments
  • Fixing bug, I was setting NameWithoutTemplate incorrectly in some cases.
Tue, Mar 3, 5:16 PM · Restricted Project
shafik added inline comments to D75488: Preserve the owning module information from DWARF in the synthesized AST.
Tue, Mar 3, 1:47 PM
shafik updated the summary of D75545: [dsymutil] Fix template stripping in getDIENames(...) to account for overloaded operators.
Tue, Mar 3, 10:56 AM · Restricted Project
shafik created D75545: [dsymutil] Fix template stripping in getDIENames(...) to account for overloaded operators.
Tue, Mar 3, 10:56 AM · Restricted Project

Feb 28 2020

shafik added a comment to D75330: [lldb] Remove checks behind LLDB_CONFIGURATION_DEBUG from TypeSystemClang.

Nice fix!

Feb 28 2020, 9:55 AM · Restricted Project

Feb 25 2020

shafik added inline comments to D75048: [ASTImporter] Improved import of AlignedAttr..
Feb 25 2020, 1:11 PM · Restricted Project

Feb 24 2020

shafik added a comment to D71018: [ASTImporter] Improved import of TypeSourceInfo (TypeLoc).

A few comments and I would like @teemperor to give this a look as well but it looks good to me.

Feb 24 2020, 2:11 PM · Restricted Project
shafik accepted D75048: [ASTImporter] Improved import of AlignedAttr..

LGTM aside from the comments I made.

Feb 24 2020, 12:57 PM · Restricted Project

Feb 21 2020

shafik accepted D74951: [lldb] Remove all the 'current_id' logging counters from the lookup code..

I have struggled to understand the real use of those counters, they may have been helpful to someone at one point but the rationale is lost in the sands of time.

Feb 21 2020, 7:09 AM · Restricted Project
shafik accepted D74957: [lldb] Disable auto fix-its when evaluating expressions in the test suite.

LGTM outside of my comments

Feb 21 2020, 7:00 AM · Restricted Project

Feb 20 2020

shafik committed rGbf3f427ba239: [ASTImporter] Add linkage check to ASTNodeImporter::hasSameVisibilityContext… (authored by shafik).
[ASTImporter] Add linkage check to ASTNodeImporter::hasSameVisibilityContext…
Feb 20 2020, 12:50 PM
shafik closed D74639: Add linkage check to ASTNodeImporter::hasSameVisibilityContext and rename to hasSameVisibilityContextAndLinkage.
Feb 20 2020, 12:50 PM · Restricted Project

Feb 14 2020

shafik added a comment to D74639: Add linkage check to ASTNodeImporter::hasSameVisibilityContext and rename to hasSameVisibilityContextAndLinkage.

I would love to have a minimal test case for this but every approach I tried has failed, so I would appreciate if there are ideas here.

Feb 14 2020, 1:16 PM · Restricted Project
shafik created D74639: Add linkage check to ASTNodeImporter::hasSameVisibilityContext and rename to hasSameVisibilityContextAndLinkage.
Feb 14 2020, 1:07 PM · Restricted Project
shafik accepted D74607: [lldb][NFC] Make all CompilerDeclContext parameters references instead of pointers.

This is a great change, it makes the code more consistent. LGTM besides the few comments I made.

Feb 14 2020, 10:48 AM · Restricted Project

Feb 13 2020

shafik abandoned D73921: Assert that a subprogram should have a name when parsing DWARF.

I am going to abandon this change b/c the consensus seems to be that we want a different solution and I don't know how much work would require ATM but I may revisit another time,

Feb 13 2020, 3:54 PM
shafik accepted D74554: [ASTImporter] Added visibility check for scoped enums..

LGTM

Feb 13 2020, 3:00 PM · Restricted Project
shafik accepted D74478: [lldb] Let TypeSystemClang::GetDisplayTypeName remove anonymous and inline namespaces..

If we still see the info using -R then I am happy but Jim's concerns are valid, they won't match the bracktrace.

Feb 13 2020, 10:43 AM · Restricted Project
shafik accepted D74542: [ASTImporter] Prevent the ASTImporter from creating multiple main FileIDs..
Feb 13 2020, 10:33 AM · Restricted Project
shafik added inline comments to D74554: [ASTImporter] Added visibility check for scoped enums..
Feb 13 2020, 10:06 AM · Restricted Project

Feb 12 2020

shafik added a comment to D74478: [lldb] Let TypeSystemClang::GetDisplayTypeName remove anonymous and inline namespaces..

I can see how stripping __1 would be nice but I seeing (anonymous namespace) may be useful to know especially b/c it effects visibility and linkage. It would be nicer if could make this optional and default it off but be able to turn it back on.

Feb 12 2020, 9:10 PM · Restricted Project

Feb 10 2020

shafik accepted D74310: [lldb] Don't model std::atomic as a transparent data structure in the data formatter.

LGTM otherwise.

Feb 10 2020, 3:10 PM · Restricted Project
shafik updated the diff for D73921: Assert that a subprogram should have a name when parsing DWARF.

Updated approach based on comments and added test for the new approach.

Feb 10 2020, 2:42 PM
shafik added a comment to D73921: Assert that a subprogram should have a name when parsing DWARF.

I will try to look into dwarfdump --verify separately.

Feb 10 2020, 2:42 PM
shafik accepted D73946: [lldb] Fix another instance where we pass a nullptr as TypeSourceInfo to NonTypeTemplateParmDecl::Create.

Thank you fixing this!

Feb 10 2020, 10:18 AM · Restricted Project

Feb 7 2020

shafik updated subscribers of D74187: [lldb] Add method Language::IsMangledName.
Feb 7 2020, 4:58 PM · Restricted Project

Feb 5 2020

shafik committed rG428583dd22fd: [DebugInfo] Fix debug-info generation for block invocations so that we set the… (authored by shafik).
[DebugInfo] Fix debug-info generation for block invocations so that we set the…
Feb 5 2020, 11:14 AM
shafik closed D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name.
Feb 5 2020, 11:14 AM · Restricted Project, debug-info

Feb 4 2020

shafik added inline comments to D73860: [lldb/StringPrinter] Avoid reading garbage in uninitialized strings.
Feb 4 2020, 11:02 PM · Restricted Project
shafik added a comment to D73969: [LLDB] Let DataExtractor deal with two-byte addresses.

Since we are doing the same test all over m_addr_size >= 1 && m_addr_size <= 8 can we just make it a function and avoid the repetition and potential erroneous updating later on that does not fix them all?

Feb 4 2020, 10:17 PM · Restricted Project, Restricted Project
shafik added a comment to D73675: Avoid many std::tie/tuple instantiations in ASTImporter.

I forgot to mention this earlier but LLDB is a major user of the ASTImporter and so in general it is a good idea to run check-lldb as well.

Feb 4 2020, 10:08 PM · Restricted Project
shafik planned changes to D73921: Assert that a subprogram should have a name when parsing DWARF.

Everyone has brought up great feedback, let me go back and revise this.

Feb 4 2020, 8:40 AM

Feb 3 2020

shafik added inline comments to D73860: [lldb/StringPrinter] Avoid reading garbage in uninitialized strings.
Feb 3 2020, 7:09 PM · Restricted Project
shafik added inline comments to D73808: [lldb/TypeSystemClang] Supply trivial TypeSourceInfo to NonTypeTemplateParmDecl::Create.
Feb 3 2020, 4:13 PM · Restricted Project
shafik added a comment to D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name.

I don't think the more targeted fix is a good thing - is the same problem of not demangling names not there for the other cases that the original fix addressed?

Feb 3 2020, 4:03 PM · Restricted Project, debug-info
shafik added inline comments to D73860: [lldb/StringPrinter] Avoid reading garbage in uninitialized strings.
Feb 3 2020, 3:34 PM · Restricted Project
shafik added a comment to D73860: [lldb/StringPrinter] Avoid reading garbage in uninitialized strings.

Is this missing additions to TestDataFormatterLibcxxString.py?

Feb 3 2020, 3:34 PM · Restricted Project