Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

ykhatav (ykhatav)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 26 2022, 11:04 AM (86 w, 3 d)

Recent Activity

Jun 7 2023

ykhatav added a comment to D144008: [DebugMetadata][DwarfDebug][CodeView] Support function-local static variables in lexical block scopes (6/7).

@akhuang @rnk @ykhatav could you please take a look at CodeViewReader.cpp change? Frankly, I am new to that format.

Thanks for taking care of this. Could you add a CodeView-specific test to make sure that the changes work as expected?

It seems that llvm/test/DebugInfo/COFF/global_visibility.ll test ensures that local/static local/global variables are handled properly. DISubprogram's retainedNodes references a few static local variables, and we check that these variables are present within CodeView dump. Is there a corner case I probably miss?

Jun 7 2023, 3:16 PM · debug-info, Restricted Project, Restricted Project
ykhatav added a comment to D144008: [DebugMetadata][DwarfDebug][CodeView] Support function-local static variables in lexical block scopes (6/7).

@akhuang @rnk @ykhatav could you please take a look at CodeViewReader.cpp change? Frankly, I am new to that format.

Jun 7 2023, 6:19 AM · debug-info, Restricted Project, Restricted Project

Mar 27 2023

ykhatav abandoned D144337: Prevent line 0 instructions from dividing a lexical block into ranges.

I will open a design discussion to discuss what an appropriate flag will be to control this behavior.

Mar 27 2023, 3:33 PM · Restricted Project, Restricted Project

Mar 16 2023

ykhatav added a comment to D144337: Prevent line 0 instructions from dividing a lexical block into ranges.

If you consider the attached test case (llvm/test/DebugInfo/X86/contiguous-lexical-block.ll) you can see that the lexical block is fragmented into ranges due to the presence of an instruction having line number 0. This I believe is a bug and If the same test case is run on windows, Visual Studio' can't reliably print variable "i" because CodeView does not provide a mechanism for describing a lexical block with more than one address ranges. In order to prevent the line 0 instructions from breaking up a lexical block, in this patch, we fold the line zero machine instructions into the range.

Sure, I'm on-board there. I'll take your word for it that CV can't represent discontiguous scopes and making scopes contiguous is the better tradeoff than omitting them entirely.

I don't have a test case that demonstrates whether using or not using UnknownLocations could lead to a bug. It is an internal flag and is suitable for controlling whether or not line zero instructions are represented in the debug information and so, in my opinion, can be used to control the scope tree as well.. Do you have any other flags in mind that can be used instead of "UnknownLocations"?

I don't understand this bit - there can be zero locations for other reasons, apart from UnknownLocations - you could just check for the zero locations, without needing to check UnknownLocations too, right?

But maybe you're using UnknownLocations as a proxy for CV/MSVC debug info emission? That seems unfortunate/not suitable, and this behavior should be gated on/more directly test for CV output, I think?

No, my intent was not to use UnknownLocations as a proxy for CV, but I think it is safe not to fold line 0 instructions into a scope range when UnknownLocations is enabled. AFAIK, when UnknownLocations is enabled line 0 entries are emitted in the debug information. In my opinion, Folding these instructions when the switch is enabled could break stuff.

@hans maybe you've got thoughts on this stuff

Mar 16 2023, 4:00 PM · Restricted Project, Restricted Project

Mar 14 2023

ykhatav added a comment to D144337: Prevent line 0 instructions from dividing a lexical block into ranges.

Could you describe why this relates to/depends on the "UnknownLocations" flag? I'm not following from the description - we could still describe these locations as being at line zero, while including them in lexical scopes to avoid fragmented ranges, I think?

The reason why we want to check for “UnknownLocations” flag is that this flag is used to check if we want to add line 0 entries in the line table (in DwarfDebug.cpp) and thereby controlling whether or not line zero instructions are represented in the debug information. If we don’t check for this flag and skip the line 0 entries while extracting lexical scopes there will be a mismatch in the line table entries and lexical scopes.

Sorry, still not quite following. Could you walk me through an example of the bug you're trying to address with this patch? And an example of the bug you're trying to avoid introducing/that causes you to need this extra UnknownLocations checking?

Mar 14 2023, 1:10 PM · Restricted Project, Restricted Project

Mar 13 2023

ykhatav added inline comments to D144337: Prevent line 0 instructions from dividing a lexical block into ranges.
Mar 13 2023, 7:46 AM · Restricted Project, Restricted Project
ykhatav updated the diff for D144337: Prevent line 0 instructions from dividing a lexical block into ranges.

Move UnknownLocations declaration to LexicalScopes.h

Mar 13 2023, 7:45 AM · Restricted Project, Restricted Project

Mar 10 2023

ykhatav added a comment to D144337: Prevent line 0 instructions from dividing a lexical block into ranges.

Could you describe why this relates to/depends on the "UnknownLocations" flag? I'm not following from the description - we could still describe these locations as being at line zero, while including them in lexical scopes to avoid fragmented ranges, I think?

The reason why we want to check for “UnknownLocations” flag is that this flag is used to check if we want to add line 0 entries in the line table (in DwarfDebug.cpp) and thereby controlling whether or not line zero instructions are represented in the debug information. If we don’t check for this flag and skip the line 0 entries while extracting lexical scopes there will be a mismatch in the line table entries and lexical scopes.

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

Mar 8 2023

ykhatav updated the diff for D144337: Prevent line 0 instructions from dividing a lexical block into ranges.

Change UnknownLocations to use boolorDefault

Mar 8 2023, 3:25 PM · Restricted Project, Restricted Project
ykhatav added inline comments to D144337: Prevent line 0 instructions from dividing a lexical block into ranges.
Mar 8 2023, 3:22 PM · Restricted Project, Restricted Project
ykhatav added a comment to D144337: Prevent line 0 instructions from dividing a lexical block into ranges.

Does this need a flag? The patch describes doing this for CodeView? Could/should it be done unconditionally for CV?

When UnknownLocations is left at default, the line table includes line 0 entries, and the lexical scopes utilize them while computing the ranges. However, if the unknown locations are disabled, the line table already ignores line 0 entries. This patch modifies the lexical scope behavior to ignore line 0 entries when UnknownLocations is not enabled. If this change is applied unconditionally, the line table entries and lexical scope will not match, causing inconsistencies
Although the DWARF format has the ability to describe the split address ranges, it is possible that the lexical blocks with split address ranges can result in buggy behavior on Linux as well.

Mar 8 2023, 3:18 PM · Restricted Project, Restricted Project

Mar 4 2023

ykhatav added a comment to D144337: Prevent line 0 instructions from dividing a lexical block into ranges.

It seems strange to define the enum in a header for a module that uses it only once, and externalize it in a module that uses it several times.

I'd always thought this DefaultOnOff business should be generalized... Hmm I see boolOrDefault in CommandLine.h. Maybe we could convert UnknownLocations to use that, remove static from the definition, and then LexicalScopes.cpp can reference it as an extern.
I agree with you that UnknownLocations may be better defined with boolorDefault instead of DefaultOnOff. I can change that.

However, the reason I moved the definition of UnknownLocations to LexicalScopes.cpp was to avoid a link-time error. Defining UnknownLocations as extern in LexicalScopes.cpp gives a link-time error despite removing the "static" keyword from its definition in DwarfDebug.cpp. I see that the two object files are in different directories and while building the LexicalScopes.cpp file it is unable to find the definition of UnknownLocations. I am aware that it may not be the optimal approach so I would appreciate any feedback or consider alternative methods.

(Converting the rest of the DwarfDebug.cpp options from DefaultOnOff to boolOrDefault should not be part of your task. But maybe add a FIXME on the enum definition.)

Mar 4 2023, 9:38 AM · Restricted Project, Restricted Project

Mar 3 2023

ykhatav added inline comments to D144008: [DebugMetadata][DwarfDebug][CodeView] Support function-local static variables in lexical block scopes (6/7).
Mar 3 2023, 8:58 AM · debug-info, Restricted Project, Restricted Project

Mar 1 2023

ykhatav added inline comments to D144008: [DebugMetadata][DwarfDebug][CodeView] Support function-local static variables in lexical block scopes (6/7).
Mar 1 2023, 6:21 AM · debug-info, Restricted Project, Restricted Project

Feb 28 2023

ykhatav added a comment to D144337: Prevent line 0 instructions from dividing a lexical block into ranges.

ping

Feb 28 2023, 7:06 AM · Restricted Project, Restricted Project

Feb 21 2023

ykhatav updated the summary of D144337: Prevent line 0 instructions from dividing a lexical block into ranges.
Feb 21 2023, 8:36 AM · Restricted Project, Restricted Project

Feb 20 2023

ykhatav updated the diff for D144337: Prevent line 0 instructions from dividing a lexical block into ranges.

Fix metadata numbering

Feb 20 2023, 6:57 AM · Restricted Project, Restricted Project
ykhatav updated the diff for D144337: Prevent line 0 instructions from dividing a lexical block into ranges.

Nit:test-fix

Feb 20 2023, 6:33 AM · Restricted Project, Restricted Project

Feb 18 2023

ykhatav requested review of D144337: Prevent line 0 instructions from dividing a lexical block into ranges.
Feb 18 2023, 3:25 PM · Restricted Project, Restricted Project

Apr 25 2022

ykhatav committed rGe83543f8c2ef: Don't replace Undef with null value for Constants Differential Revision:https… (authored by ykhatav).
Don't replace Undef with null value for Constants Differential Revision:https…
Apr 25 2022, 5:50 PM · Restricted Project, Restricted Project
ykhatav updated the diff for D124098: Don't replace Undef with null value for Constants.
Apr 25 2022, 9:33 AM · Restricted Project, Restricted Project
ykhatav updated the diff for D124098: Don't replace Undef with null value for Constants.

Removed the attribute

Apr 25 2022, 9:13 AM · Restricted Project, Restricted Project

Apr 22 2022

ykhatav added reviewers for D124098: Don't replace Undef with null value for Constants: echristo, aprantl.
Apr 22 2022, 9:20 AM · Restricted Project, Restricted Project
ykhatav closed D118276: Adding a DIBuilder interface for Fortran's assumed length string.

Commit id: 70fdbf35de1c37a22a5321ed7d7d2c190d788549

Apr 22 2022, 7:56 AM · Restricted Project, Restricted Project

Apr 20 2022

ykhatav requested review of D124098: Don't replace Undef with null value for Constants.
Apr 20 2022, 8:59 AM · Restricted Project, Restricted Project

Feb 11 2022

ykhatav committed rGf9f78a2c408a: Fix build broken by missing empty line in SourceLevelDebugging.rst (authored by ykhatav).
Fix build broken by missing empty line in SourceLevelDebugging.rst
Feb 11 2022, 12:19 PM
ykhatav committed rG70fdbf35de1c: Adding DiBuilder interface for assumed length strings (authored by ykhatav).
Adding DiBuilder interface for assumed length strings
Feb 11 2022, 11:40 AM

Feb 10 2022

ykhatav committed rG93d1a623cecb: Reverting an entire stack of changes causing build failures (authored by ykhatav).
Reverting an entire stack of changes causing build failures
Feb 10 2022, 2:59 PM
ykhatav committed rGac15cd7af615: Modified SourceLevelDebugging.rst to include information about memory location… (authored by ykhatav).
Modified SourceLevelDebugging.rst to include information about memory location…
Feb 10 2022, 12:26 PM
ykhatav committed rGe4f9d4a5eee3: updated local branch to incorporate latest changes (authored by ykhatav).
updated local branch to incorporate latest changes
Feb 10 2022, 12:26 PM
ykhatav committed rG0e7341b7b199: worked on review comments (authored by ykhatav).
worked on review comments
Feb 10 2022, 12:26 PM
ykhatav committed rGc26a0d1cda29: Updated the test to include proper string get functions (authored by ykhatav).
Updated the test to include proper string get functions
Feb 10 2022, 12:26 PM
ykhatav committed rG929499eb641e: Updated the test to include addtional details (authored by ykhatav).
Updated the test to include addtional details
Feb 10 2022, 12:26 PM
ykhatav committed rG99f990be6480: Added StringLocationExp to the new apis (authored by ykhatav).
Added StringLocationExp to the new apis
Feb 10 2022, 12:26 PM
ykhatav committed rG2c5dfeed2f77: Addressed review comments (authored by ykhatav).
Addressed review comments
Feb 10 2022, 12:25 PM
ykhatav committed rG43d421cda395: Adding DIBuilder interface for assumed length string (authored by ykhatav).
Adding DIBuilder interface for assumed length string
Feb 10 2022, 12:25 PM

Feb 7 2022

ykhatav updated the diff for D118276: Adding a DIBuilder interface for Fortran's assumed length string.

Modified SourceLevelDebugging.rst to include information about memory location exp.

Feb 7 2022, 7:06 AM · Restricted Project, Restricted Project

Feb 3 2022

ykhatav updated the diff for D118276: Adding a DIBuilder interface for Fortran's assumed length string.
Feb 3 2022, 5:49 AM · Restricted Project, Restricted Project

Feb 2 2022

ykhatav updated the diff for D118276: Adding a DIBuilder interface for Fortran's assumed length string.

Updated my local branch with the latest changes

Feb 2 2022, 12:51 PM · Restricted Project, Restricted Project

Feb 1 2022

ykhatav updated the diff for D118276: Adding a DIBuilder interface for Fortran's assumed length string.

Worked on review comments

Feb 1 2022, 12:47 PM · Restricted Project, Restricted Project
ykhatav updated the diff for D118276: Adding a DIBuilder interface for Fortran's assumed length string.

Updated the test to include the proper string functions

Feb 1 2022, 7:10 AM · Restricted Project, Restricted Project

Jan 31 2022

ykhatav added a reviewer for D118276: Adding a DIBuilder interface for Fortran's assumed length string: aprantl.
Jan 31 2022, 10:07 AM · Restricted Project, Restricted Project
ykhatav updated the diff for D118276: Adding a DIBuilder interface for Fortran's assumed length string.
Jan 31 2022, 8:50 AM · Restricted Project, Restricted Project

Jan 27 2022

ykhatav updated the diff for D118276: Adding a DIBuilder interface for Fortran's assumed length string.

Added StringLocationExp to the new apis

Jan 27 2022, 11:23 AM · Restricted Project, Restricted Project

Jan 26 2022

ykhatav requested review of D118276: Adding a DIBuilder interface for Fortran's assumed length string.
Jan 26 2022, 12:06 PM · Restricted Project, Restricted Project