Remove printing of DacpModuleData::PEAssembly from DumpModule command by elinor-fung · Pull Request #4751 · dotnet/diagnostics

elinor-fung · 2024-06-22T02:39:28Z

dotnet/runtime#103821 changes the PEAssembly field to actually be the Module.

Per dotnet/runtime#103821 (comment), remove printing it to avoid confusion.

jkotas · 2024-06-22T03:15:54Z

Tests may need updating

leculver

SOS supports both Desktop Framework and .Net Core. Merging this change would make Desktop Framework debugging worse. (Also this code still needs to work on .Net 7 and 8 while they are still in support.)

Instead, you should test whether .PEAssembly is equal to .Address and conditionally print it to preserve debugging the other runtime.

jkotas · 2024-06-23T01:44:10Z

Merging this change would make Desktop Framework debugging worse.

IMHO, this change makes the .NET Framework debugging better and less confusing. The value that this line prints on .NET Framework is actually Module's PEFile, that may or may not be PEAssembly. And if somebody wants to look at PEFile, it is a simple field in the Module type. It is trivial to fetch it using regular windbg debugger commands.

I do not have strong opinions about what this should do on .NET Framework. I am fine with keeping the .NET Framework behavior intact with all its quirks.

you should test whether .PEAssembly is equal to .Address and conditionally print it to preserve debugging the other runtime.

I like the idea of deleting clutter from the output of these commands. We can also do the same for module.dwModuleID and module.dwModuleIndex and print them only when they are non-zero. These concepts do not exist anymore. The DAC returns constant 0 for these fields: https://github.com/dotnet/runtime/blob/8e92aef5387fe1d4b9159b4a3657416ac7d0a05a/src/coreclr/debug/daccess/request.cpp#L1738-L1739.

leculver · 2024-06-23T03:37:17Z

And if somebody wants to look at PEFile, it is a simple field in the Module type. It is trivial to fetch it using regular windbg debugger commands.

We don't ship private symbols for desktop framework. This is something SOS/dac produces without having them.

I'm also in favor of removing clutter from commands, like displaying dwModuleID/dwModuleIndex only when non-zero. I just don't want to remove potentially useful bits for Desktop Framework as an accidental biproduct of changes here when avoidable.

jkotas · 2024-06-23T03:46:01Z

We don't ship private symbols for desktop framework. This is something SOS/dac produces without having them.

Can you do anything useful with PEAssembly pointer if you do not have private symbols? I think PEAssembly pointer is completely useless without private symbols.

leculver · 2024-06-23T15:49:15Z

I don't know all of the use cases of SOS. I'd simply prefer not to remove functionality if we don't have to.

You can still implement all of the .Net Core related functionality here without affecting Desktop Framework debugging.

leculver · 2024-06-23T16:59:34Z

Stepping back a second, the principle I'm trying to articulate is this:

When possible, please don't regress or take back Desktop Framework debugging features, as some of us still have to regularly debug that product. (Of course, I also mean when it wouldn't be too much work. I am not trying to hold anyone back.) As long as this version of SOS continues to support Desktop Framework, I think that's a reasonable position to take.

In this particular case, you are still able to make the changes you want to the output...it only means adding an if statement to maintain the status quo for Desktop. Feel free to change the text of "PEAssembly" to something more correct, always happy to have improvments. :)

I think that's a pretty reasonable (and not onerous) request, regardless of how useful the functionality is. Much appreciated!

jkotas · 2024-06-23T17:13:27Z

When possible, please don't regress or take back Desktop Framework debugging feature

I agree.

Feel free to change the text of "PEAssembly" to something more correct, always happy to have improvements. :)

Well, I am saying that the improvement would be to delete it unless somebody can explain why it is useful. This information was not in DumpModule output in the .NET Framework golden days and nobody missed it enough to add it to the output.

It was added to DumpModule output recently as part of 3000+ lines change that added support for ELF dumps: #124 . I am not sure why Mike added it. I do not see any paper trail that explains why it is useful to have this information in the DumpModule output.

mikem8361 · 2024-06-23T17:17:49Z

I don't remember why I added it of course. I'm completely ok with making these kind of SOS improvements.

mikem8361 · 2024-06-23T17:22:57Z

The SOS tests are failing because they expect a PEAssembly in the output. Line 91 in src\SOS\SOS.UnitTests\Scripts\OtherCommands.script needs to be removed.

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

elinor-fung · 2024-06-24T15:49:12Z

@leculver - completely agree with the higher level principle around Framework debugging experience and appreciate you calling it out here. For this particular case, per #4751 (comment) and #4751 (comment), this was a relatively recent addition that was never part of the original Framework experience, so I kept this PR as unconditionally removing the output.

elinor-fung · 2024-06-22T02:39:28Z

dotnet/runtime#103821 changes the PEAssembly field to actually be the Module.

Per dotnet/runtime#103821 (comment), remove printing it to avoid confusion.

jkotas · 2024-06-22T03:15:54Z

Tests may need updating

leculver

SOS supports both Desktop Framework and .Net Core. Merging this change would make Desktop Framework debugging worse. (Also this code still needs to work on .Net 7 and 8 while they are still in support.)

Instead, you should test whether .PEAssembly is equal to .Address and conditionally print it to preserve debugging the other runtime.

jkotas · 2024-06-23T01:44:10Z

Merging this change would make Desktop Framework debugging worse.

IMHO, this change makes the .NET Framework debugging better and less confusing. The value that this line prints on .NET Framework is actually Module's PEFile, that may or may not be PEAssembly. And if somebody wants to look at PEFile, it is a simple field in the Module type. It is trivial to fetch it using regular windbg debugger commands.

I do not have strong opinions about what this should do on .NET Framework. I am fine with keeping the .NET Framework behavior intact with all its quirks.

you should test whether .PEAssembly is equal to .Address and conditionally print it to preserve debugging the other runtime.

I like the idea of deleting clutter from the output of these commands. We can also do the same for module.dwModuleID and module.dwModuleIndex and print them only when they are non-zero. These concepts do not exist anymore. The DAC returns constant 0 for these fields: https://github.com/dotnet/runtime/blob/8e92aef5387fe1d4b9159b4a3657416ac7d0a05a/src/coreclr/debug/daccess/request.cpp#L1738-L1739.

leculver · 2024-06-23T03:37:17Z

And if somebody wants to look at PEFile, it is a simple field in the Module type. It is trivial to fetch it using regular windbg debugger commands.

We don't ship private symbols for desktop framework. This is something SOS/dac produces without having them.

I'm also in favor of removing clutter from commands, like displaying dwModuleID/dwModuleIndex only when non-zero. I just don't want to remove potentially useful bits for Desktop Framework as an accidental biproduct of changes here when avoidable.

jkotas · 2024-06-23T03:46:01Z

We don't ship private symbols for desktop framework. This is something SOS/dac produces without having them.

Can you do anything useful with PEAssembly pointer if you do not have private symbols? I think PEAssembly pointer is completely useless without private symbols.

leculver · 2024-06-23T15:49:15Z

I don't know all of the use cases of SOS. I'd simply prefer not to remove functionality if we don't have to.

You can still implement all of the .Net Core related functionality here without affecting Desktop Framework debugging.

leculver · 2024-06-23T16:59:34Z

Stepping back a second, the principle I'm trying to articulate is this:

When possible, please don't regress or take back Desktop Framework debugging features, as some of us still have to regularly debug that product. (Of course, I also mean when it wouldn't be too much work. I am not trying to hold anyone back.) As long as this version of SOS continues to support Desktop Framework, I think that's a reasonable position to take.

In this particular case, you are still able to make the changes you want to the output...it only means adding an if statement to maintain the status quo for Desktop. Feel free to change the text of "PEAssembly" to something more correct, always happy to have improvments. :)

I think that's a pretty reasonable (and not onerous) request, regardless of how useful the functionality is. Much appreciated!

jkotas · 2024-06-23T17:13:27Z

When possible, please don't regress or take back Desktop Framework debugging feature

I agree.

Feel free to change the text of "PEAssembly" to something more correct, always happy to have improvements. :)

Well, I am saying that the improvement would be to delete it unless somebody can explain why it is useful. This information was not in DumpModule output in the .NET Framework golden days and nobody missed it enough to add it to the output.

It was added to DumpModule output recently as part of 3000+ lines change that added support for ELF dumps: #124 . I am not sure why Mike added it. I do not see any paper trail that explains why it is useful to have this information in the DumpModule output.

mikem8361 · 2024-06-23T17:17:49Z

I don't remember why I added it of course. I'm completely ok with making these kind of SOS improvements.

mikem8361 · 2024-06-23T17:22:57Z

The SOS tests are failing because they expect a PEAssembly in the output. Line 91 in src\SOS\SOS.UnitTests\Scripts\OtherCommands.script needs to be removed.

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

elinor-fung · 2024-06-24T15:49:12Z

@leculver - completely agree with the higher level principle around Framework debugging experience and appreciate you calling it out here. For this particular case, per #4751 (comment) and #4751 (comment), this was a relatively recent addition that was never part of the original Framework experience, so I kept this PR as unconditionally removing the output.

elinor-fung · 2024-06-22T02:39:28Z

dotnet/runtime#103821 changes the PEAssembly field to actually be the Module.

Per dotnet/runtime#103821 (comment), remove printing it to avoid confusion.

jkotas · 2024-06-22T03:15:54Z

Tests may need updating

leculver

SOS supports both Desktop Framework and .Net Core. Merging this change would make Desktop Framework debugging worse. (Also this code still needs to work on .Net 7 and 8 while they are still in support.)

Instead, you should test whether .PEAssembly is equal to .Address and conditionally print it to preserve debugging the other runtime.

jkotas · 2024-06-23T01:44:10Z

Merging this change would make Desktop Framework debugging worse.

IMHO, this change makes the .NET Framework debugging better and less confusing. The value that this line prints on .NET Framework is actually Module's PEFile, that may or may not be PEAssembly. And if somebody wants to look at PEFile, it is a simple field in the Module type. It is trivial to fetch it using regular windbg debugger commands.

I do not have strong opinions about what this should do on .NET Framework. I am fine with keeping the .NET Framework behavior intact with all its quirks.

you should test whether .PEAssembly is equal to .Address and conditionally print it to preserve debugging the other runtime.

I like the idea of deleting clutter from the output of these commands. We can also do the same for module.dwModuleID and module.dwModuleIndex and print them only when they are non-zero. These concepts do not exist anymore. The DAC returns constant 0 for these fields: https://github.com/dotnet/runtime/blob/8e92aef5387fe1d4b9159b4a3657416ac7d0a05a/src/coreclr/debug/daccess/request.cpp#L1738-L1739.

leculver · 2024-06-23T03:37:17Z

And if somebody wants to look at PEFile, it is a simple field in the Module type. It is trivial to fetch it using regular windbg debugger commands.

We don't ship private symbols for desktop framework. This is something SOS/dac produces without having them.

I'm also in favor of removing clutter from commands, like displaying dwModuleID/dwModuleIndex only when non-zero. I just don't want to remove potentially useful bits for Desktop Framework as an accidental biproduct of changes here when avoidable.

jkotas · 2024-06-23T03:46:01Z

We don't ship private symbols for desktop framework. This is something SOS/dac produces without having them.

Can you do anything useful with PEAssembly pointer if you do not have private symbols? I think PEAssembly pointer is completely useless without private symbols.

leculver · 2024-06-23T15:49:15Z

I don't know all of the use cases of SOS. I'd simply prefer not to remove functionality if we don't have to.

You can still implement all of the .Net Core related functionality here without affecting Desktop Framework debugging.

leculver · 2024-06-23T16:59:34Z

Stepping back a second, the principle I'm trying to articulate is this:

When possible, please don't regress or take back Desktop Framework debugging features, as some of us still have to regularly debug that product. (Of course, I also mean when it wouldn't be too much work. I am not trying to hold anyone back.) As long as this version of SOS continues to support Desktop Framework, I think that's a reasonable position to take.

In this particular case, you are still able to make the changes you want to the output...it only means adding an if statement to maintain the status quo for Desktop. Feel free to change the text of "PEAssembly" to something more correct, always happy to have improvments. :)

I think that's a pretty reasonable (and not onerous) request, regardless of how useful the functionality is. Much appreciated!

jkotas · 2024-06-23T17:13:27Z

When possible, please don't regress or take back Desktop Framework debugging feature

I agree.

Feel free to change the text of "PEAssembly" to something more correct, always happy to have improvements. :)

Well, I am saying that the improvement would be to delete it unless somebody can explain why it is useful. This information was not in DumpModule output in the .NET Framework golden days and nobody missed it enough to add it to the output.

It was added to DumpModule output recently as part of 3000+ lines change that added support for ELF dumps: #124 . I am not sure why Mike added it. I do not see any paper trail that explains why it is useful to have this information in the DumpModule output.

mikem8361 · 2024-06-23T17:17:49Z

I don't remember why I added it of course. I'm completely ok with making these kind of SOS improvements.

mikem8361 · 2024-06-23T17:22:57Z

The SOS tests are failing because they expect a PEAssembly in the output. Line 91 in src\SOS\SOS.UnitTests\Scripts\OtherCommands.script needs to be removed.

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

elinor-fung · 2024-06-24T15:49:12Z

@leculver - completely agree with the higher level principle around Framework debugging experience and appreciate you calling it out here. For this particular case, per #4751 (comment) and #4751 (comment), this was a relatively recent addition that was never part of the original Framework experience, so I kept this PR as unconditionally removing the output.

elinor-fung · 2024-06-22T02:39:28Z

dotnet/runtime#103821 changes the PEAssembly field to actually be the Module.

Per dotnet/runtime#103821 (comment), remove printing it to avoid confusion.

jkotas · 2024-06-22T03:15:54Z

Tests may need updating

leculver

SOS supports both Desktop Framework and .Net Core. Merging this change would make Desktop Framework debugging worse. (Also this code still needs to work on .Net 7 and 8 while they are still in support.)

Instead, you should test whether .PEAssembly is equal to .Address and conditionally print it to preserve debugging the other runtime.

jkotas · 2024-06-23T01:44:10Z

Merging this change would make Desktop Framework debugging worse.

IMHO, this change makes the .NET Framework debugging better and less confusing. The value that this line prints on .NET Framework is actually Module's PEFile, that may or may not be PEAssembly. And if somebody wants to look at PEFile, it is a simple field in the Module type. It is trivial to fetch it using regular windbg debugger commands.

I do not have strong opinions about what this should do on .NET Framework. I am fine with keeping the .NET Framework behavior intact with all its quirks.

you should test whether .PEAssembly is equal to .Address and conditionally print it to preserve debugging the other runtime.

I like the idea of deleting clutter from the output of these commands. We can also do the same for module.dwModuleID and module.dwModuleIndex and print them only when they are non-zero. These concepts do not exist anymore. The DAC returns constant 0 for these fields: https://github.com/dotnet/runtime/blob/8e92aef5387fe1d4b9159b4a3657416ac7d0a05a/src/coreclr/debug/daccess/request.cpp#L1738-L1739.

leculver · 2024-06-23T03:37:17Z

And if somebody wants to look at PEFile, it is a simple field in the Module type. It is trivial to fetch it using regular windbg debugger commands.

We don't ship private symbols for desktop framework. This is something SOS/dac produces without having them.

I'm also in favor of removing clutter from commands, like displaying dwModuleID/dwModuleIndex only when non-zero. I just don't want to remove potentially useful bits for Desktop Framework as an accidental biproduct of changes here when avoidable.

jkotas · 2024-06-23T03:46:01Z

We don't ship private symbols for desktop framework. This is something SOS/dac produces without having them.

Can you do anything useful with PEAssembly pointer if you do not have private symbols? I think PEAssembly pointer is completely useless without private symbols.

leculver · 2024-06-23T15:49:15Z

I don't know all of the use cases of SOS. I'd simply prefer not to remove functionality if we don't have to.

You can still implement all of the .Net Core related functionality here without affecting Desktop Framework debugging.

leculver · 2024-06-23T16:59:34Z

Stepping back a second, the principle I'm trying to articulate is this:

When possible, please don't regress or take back Desktop Framework debugging features, as some of us still have to regularly debug that product. (Of course, I also mean when it wouldn't be too much work. I am not trying to hold anyone back.) As long as this version of SOS continues to support Desktop Framework, I think that's a reasonable position to take.

In this particular case, you are still able to make the changes you want to the output...it only means adding an if statement to maintain the status quo for Desktop. Feel free to change the text of "PEAssembly" to something more correct, always happy to have improvments. :)

I think that's a pretty reasonable (and not onerous) request, regardless of how useful the functionality is. Much appreciated!

jkotas · 2024-06-23T17:13:27Z

When possible, please don't regress or take back Desktop Framework debugging feature

I agree.

Feel free to change the text of "PEAssembly" to something more correct, always happy to have improvements. :)

Well, I am saying that the improvement would be to delete it unless somebody can explain why it is useful. This information was not in DumpModule output in the .NET Framework golden days and nobody missed it enough to add it to the output.

It was added to DumpModule output recently as part of 3000+ lines change that added support for ELF dumps: #124 . I am not sure why Mike added it. I do not see any paper trail that explains why it is useful to have this information in the DumpModule output.

mikem8361 · 2024-06-23T17:17:49Z

I don't remember why I added it of course. I'm completely ok with making these kind of SOS improvements.

mikem8361 · 2024-06-23T17:22:57Z

The SOS tests are failing because they expect a PEAssembly in the output. Line 91 in src\SOS\SOS.UnitTests\Scripts\OtherCommands.script needs to be removed.

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

elinor-fung · 2024-06-24T15:49:12Z

@leculver - completely agree with the higher level principle around Framework debugging experience and appreciate you calling it out here. For this particular case, per #4751 (comment) and #4751 (comment), this was a relatively recent addition that was never part of the original Framework experience, so I kept this PR as unconditionally removing the output.

elinor-fung · 2024-06-22T02:39:28Z

dotnet/runtime#103821 changes the PEAssembly field to actually be the Module.

Per dotnet/runtime#103821 (comment), remove printing it to avoid confusion.

jkotas · 2024-06-22T03:15:54Z

Tests may need updating

leculver

SOS supports both Desktop Framework and .Net Core. Merging this change would make Desktop Framework debugging worse. (Also this code still needs to work on .Net 7 and 8 while they are still in support.)

Instead, you should test whether .PEAssembly is equal to .Address and conditionally print it to preserve debugging the other runtime.

jkotas · 2024-06-23T01:44:10Z

Merging this change would make Desktop Framework debugging worse.

IMHO, this change makes the .NET Framework debugging better and less confusing. The value that this line prints on .NET Framework is actually Module's PEFile, that may or may not be PEAssembly. And if somebody wants to look at PEFile, it is a simple field in the Module type. It is trivial to fetch it using regular windbg debugger commands.

I do not have strong opinions about what this should do on .NET Framework. I am fine with keeping the .NET Framework behavior intact with all its quirks.

you should test whether .PEAssembly is equal to .Address and conditionally print it to preserve debugging the other runtime.

I like the idea of deleting clutter from the output of these commands. We can also do the same for module.dwModuleID and module.dwModuleIndex and print them only when they are non-zero. These concepts do not exist anymore. The DAC returns constant 0 for these fields: https://github.com/dotnet/runtime/blob/8e92aef5387fe1d4b9159b4a3657416ac7d0a05a/src/coreclr/debug/daccess/request.cpp#L1738-L1739.

leculver · 2024-06-23T03:37:17Z

And if somebody wants to look at PEFile, it is a simple field in the Module type. It is trivial to fetch it using regular windbg debugger commands.

We don't ship private symbols for desktop framework. This is something SOS/dac produces without having them.

I'm also in favor of removing clutter from commands, like displaying dwModuleID/dwModuleIndex only when non-zero. I just don't want to remove potentially useful bits for Desktop Framework as an accidental biproduct of changes here when avoidable.

jkotas · 2024-06-23T03:46:01Z

We don't ship private symbols for desktop framework. This is something SOS/dac produces without having them.

Can you do anything useful with PEAssembly pointer if you do not have private symbols? I think PEAssembly pointer is completely useless without private symbols.

leculver · 2024-06-23T15:49:15Z

I don't know all of the use cases of SOS. I'd simply prefer not to remove functionality if we don't have to.

You can still implement all of the .Net Core related functionality here without affecting Desktop Framework debugging.

leculver · 2024-06-23T16:59:34Z

Stepping back a second, the principle I'm trying to articulate is this:

When possible, please don't regress or take back Desktop Framework debugging features, as some of us still have to regularly debug that product. (Of course, I also mean when it wouldn't be too much work. I am not trying to hold anyone back.) As long as this version of SOS continues to support Desktop Framework, I think that's a reasonable position to take.

In this particular case, you are still able to make the changes you want to the output...it only means adding an if statement to maintain the status quo for Desktop. Feel free to change the text of "PEAssembly" to something more correct, always happy to have improvments. :)

I think that's a pretty reasonable (and not onerous) request, regardless of how useful the functionality is. Much appreciated!

jkotas · 2024-06-23T17:13:27Z

When possible, please don't regress or take back Desktop Framework debugging feature

I agree.

Feel free to change the text of "PEAssembly" to something more correct, always happy to have improvements. :)

Well, I am saying that the improvement would be to delete it unless somebody can explain why it is useful. This information was not in DumpModule output in the .NET Framework golden days and nobody missed it enough to add it to the output.

It was added to DumpModule output recently as part of 3000+ lines change that added support for ELF dumps: #124 . I am not sure why Mike added it. I do not see any paper trail that explains why it is useful to have this information in the DumpModule output.

mikem8361 · 2024-06-23T17:17:49Z

I don't remember why I added it of course. I'm completely ok with making these kind of SOS improvements.

mikem8361 · 2024-06-23T17:22:57Z

The SOS tests are failing because they expect a PEAssembly in the output. Line 91 in src\SOS\SOS.UnitTests\Scripts\OtherCommands.script needs to be removed.

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

elinor-fung · 2024-06-24T15:49:12Z

@leculver - completely agree with the higher level principle around Framework debugging experience and appreciate you calling it out here. For this particular case, per #4751 (comment) and #4751 (comment), this was a relatively recent addition that was never part of the original Framework experience, so I kept this PR as unconditionally removing the output.

elinor-fung · 2024-06-22T02:39:28Z

dotnet/runtime#103821 changes the PEAssembly field to actually be the Module.

Per dotnet/runtime#103821 (comment), remove printing it to avoid confusion.

jkotas · 2024-06-22T03:15:54Z

Tests may need updating

leculver

SOS supports both Desktop Framework and .Net Core. Merging this change would make Desktop Framework debugging worse. (Also this code still needs to work on .Net 7 and 8 while they are still in support.)

Instead, you should test whether .PEAssembly is equal to .Address and conditionally print it to preserve debugging the other runtime.

jkotas · 2024-06-23T01:44:10Z

Merging this change would make Desktop Framework debugging worse.

IMHO, this change makes the .NET Framework debugging better and less confusing. The value that this line prints on .NET Framework is actually Module's PEFile, that may or may not be PEAssembly. And if somebody wants to look at PEFile, it is a simple field in the Module type. It is trivial to fetch it using regular windbg debugger commands.

I do not have strong opinions about what this should do on .NET Framework. I am fine with keeping the .NET Framework behavior intact with all its quirks.

you should test whether .PEAssembly is equal to .Address and conditionally print it to preserve debugging the other runtime.

I like the idea of deleting clutter from the output of these commands. We can also do the same for module.dwModuleID and module.dwModuleIndex and print them only when they are non-zero. These concepts do not exist anymore. The DAC returns constant 0 for these fields: https://github.com/dotnet/runtime/blob/8e92aef5387fe1d4b9159b4a3657416ac7d0a05a/src/coreclr/debug/daccess/request.cpp#L1738-L1739.

leculver · 2024-06-23T03:37:17Z

And if somebody wants to look at PEFile, it is a simple field in the Module type. It is trivial to fetch it using regular windbg debugger commands.

We don't ship private symbols for desktop framework. This is something SOS/dac produces without having them.

I'm also in favor of removing clutter from commands, like displaying dwModuleID/dwModuleIndex only when non-zero. I just don't want to remove potentially useful bits for Desktop Framework as an accidental biproduct of changes here when avoidable.

jkotas · 2024-06-23T03:46:01Z

We don't ship private symbols for desktop framework. This is something SOS/dac produces without having them.

Can you do anything useful with PEAssembly pointer if you do not have private symbols? I think PEAssembly pointer is completely useless without private symbols.

leculver · 2024-06-23T15:49:15Z

I don't know all of the use cases of SOS. I'd simply prefer not to remove functionality if we don't have to.

You can still implement all of the .Net Core related functionality here without affecting Desktop Framework debugging.

leculver · 2024-06-23T16:59:34Z

Stepping back a second, the principle I'm trying to articulate is this:

When possible, please don't regress or take back Desktop Framework debugging features, as some of us still have to regularly debug that product. (Of course, I also mean when it wouldn't be too much work. I am not trying to hold anyone back.) As long as this version of SOS continues to support Desktop Framework, I think that's a reasonable position to take.

In this particular case, you are still able to make the changes you want to the output...it only means adding an if statement to maintain the status quo for Desktop. Feel free to change the text of "PEAssembly" to something more correct, always happy to have improvments. :)

I think that's a pretty reasonable (and not onerous) request, regardless of how useful the functionality is. Much appreciated!

jkotas · 2024-06-23T17:13:27Z

When possible, please don't regress or take back Desktop Framework debugging feature

I agree.

Feel free to change the text of "PEAssembly" to something more correct, always happy to have improvements. :)

Well, I am saying that the improvement would be to delete it unless somebody can explain why it is useful. This information was not in DumpModule output in the .NET Framework golden days and nobody missed it enough to add it to the output.

It was added to DumpModule output recently as part of 3000+ lines change that added support for ELF dumps: #124 . I am not sure why Mike added it. I do not see any paper trail that explains why it is useful to have this information in the DumpModule output.

mikem8361 · 2024-06-23T17:17:49Z

I don't remember why I added it of course. I'm completely ok with making these kind of SOS improvements.

mikem8361 · 2024-06-23T17:22:57Z

The SOS tests are failing because they expect a PEAssembly in the output. Line 91 in src\SOS\SOS.UnitTests\Scripts\OtherCommands.script needs to be removed.

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

elinor-fung · 2024-06-24T15:49:12Z

@leculver - completely agree with the higher level principle around Framework debugging experience and appreciate you calling it out here. For this particular case, per #4751 (comment) and #4751 (comment), this was a relatively recent addition that was never part of the original Framework experience, so I kept this PR as unconditionally removing the output.

elinor-fung · 2024-06-22T02:39:28Z

dotnet/runtime#103821 changes the PEAssembly field to actually be the Module.

Per dotnet/runtime#103821 (comment), remove printing it to avoid confusion.

jkotas · 2024-06-22T03:15:54Z

Tests may need updating

leculver

SOS supports both Desktop Framework and .Net Core. Merging this change would make Desktop Framework debugging worse. (Also this code still needs to work on .Net 7 and 8 while they are still in support.)

Instead, you should test whether .PEAssembly is equal to .Address and conditionally print it to preserve debugging the other runtime.

jkotas · 2024-06-23T01:44:10Z

Merging this change would make Desktop Framework debugging worse.

IMHO, this change makes the .NET Framework debugging better and less confusing. The value that this line prints on .NET Framework is actually Module's PEFile, that may or may not be PEAssembly. And if somebody wants to look at PEFile, it is a simple field in the Module type. It is trivial to fetch it using regular windbg debugger commands.

I do not have strong opinions about what this should do on .NET Framework. I am fine with keeping the .NET Framework behavior intact with all its quirks.

you should test whether .PEAssembly is equal to .Address and conditionally print it to preserve debugging the other runtime.

I like the idea of deleting clutter from the output of these commands. We can also do the same for module.dwModuleID and module.dwModuleIndex and print them only when they are non-zero. These concepts do not exist anymore. The DAC returns constant 0 for these fields: https://github.com/dotnet/runtime/blob/8e92aef5387fe1d4b9159b4a3657416ac7d0a05a/src/coreclr/debug/daccess/request.cpp#L1738-L1739.

leculver · 2024-06-23T03:37:17Z

And if somebody wants to look at PEFile, it is a simple field in the Module type. It is trivial to fetch it using regular windbg debugger commands.

We don't ship private symbols for desktop framework. This is something SOS/dac produces without having them.

I'm also in favor of removing clutter from commands, like displaying dwModuleID/dwModuleIndex only when non-zero. I just don't want to remove potentially useful bits for Desktop Framework as an accidental biproduct of changes here when avoidable.

jkotas · 2024-06-23T03:46:01Z

We don't ship private symbols for desktop framework. This is something SOS/dac produces without having them.

Can you do anything useful with PEAssembly pointer if you do not have private symbols? I think PEAssembly pointer is completely useless without private symbols.

leculver · 2024-06-23T15:49:15Z

I don't know all of the use cases of SOS. I'd simply prefer not to remove functionality if we don't have to.

You can still implement all of the .Net Core related functionality here without affecting Desktop Framework debugging.

leculver · 2024-06-23T16:59:34Z

Stepping back a second, the principle I'm trying to articulate is this:

When possible, please don't regress or take back Desktop Framework debugging features, as some of us still have to regularly debug that product. (Of course, I also mean when it wouldn't be too much work. I am not trying to hold anyone back.) As long as this version of SOS continues to support Desktop Framework, I think that's a reasonable position to take.

In this particular case, you are still able to make the changes you want to the output...it only means adding an if statement to maintain the status quo for Desktop. Feel free to change the text of "PEAssembly" to something more correct, always happy to have improvments. :)

I think that's a pretty reasonable (and not onerous) request, regardless of how useful the functionality is. Much appreciated!

jkotas · 2024-06-23T17:13:27Z

When possible, please don't regress or take back Desktop Framework debugging feature

I agree.

Feel free to change the text of "PEAssembly" to something more correct, always happy to have improvements. :)

Well, I am saying that the improvement would be to delete it unless somebody can explain why it is useful. This information was not in DumpModule output in the .NET Framework golden days and nobody missed it enough to add it to the output.

It was added to DumpModule output recently as part of 3000+ lines change that added support for ELF dumps: #124 . I am not sure why Mike added it. I do not see any paper trail that explains why it is useful to have this information in the DumpModule output.

mikem8361 · 2024-06-23T17:17:49Z

I don't remember why I added it of course. I'm completely ok with making these kind of SOS improvements.

mikem8361 · 2024-06-23T17:22:57Z

The SOS tests are failing because they expect a PEAssembly in the output. Line 91 in src\SOS\SOS.UnitTests\Scripts\OtherCommands.script needs to be removed.

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

elinor-fung · 2024-06-24T15:49:12Z

@leculver - completely agree with the higher level principle around Framework debugging experience and appreciate you calling it out here. For this particular case, per #4751 (comment) and #4751 (comment), this was a relatively recent addition that was never part of the original Framework experience, so I kept this PR as unconditionally removing the output.

elinor-fung · 2024-06-22T02:39:28Z

dotnet/runtime#103821 changes the PEAssembly field to actually be the Module.

Per dotnet/runtime#103821 (comment), remove printing it to avoid confusion.

jkotas · 2024-06-22T03:15:54Z

Tests may need updating

leculver

SOS supports both Desktop Framework and .Net Core. Merging this change would make Desktop Framework debugging worse. (Also this code still needs to work on .Net 7 and 8 while they are still in support.)

Instead, you should test whether .PEAssembly is equal to .Address and conditionally print it to preserve debugging the other runtime.

jkotas · 2024-06-23T01:44:10Z

Merging this change would make Desktop Framework debugging worse.

IMHO, this change makes the .NET Framework debugging better and less confusing. The value that this line prints on .NET Framework is actually Module's PEFile, that may or may not be PEAssembly. And if somebody wants to look at PEFile, it is a simple field in the Module type. It is trivial to fetch it using regular windbg debugger commands.

I do not have strong opinions about what this should do on .NET Framework. I am fine with keeping the .NET Framework behavior intact with all its quirks.

you should test whether .PEAssembly is equal to .Address and conditionally print it to preserve debugging the other runtime.

I like the idea of deleting clutter from the output of these commands. We can also do the same for module.dwModuleID and module.dwModuleIndex and print them only when they are non-zero. These concepts do not exist anymore. The DAC returns constant 0 for these fields: https://github.com/dotnet/runtime/blob/8e92aef5387fe1d4b9159b4a3657416ac7d0a05a/src/coreclr/debug/daccess/request.cpp#L1738-L1739.

leculver · 2024-06-23T03:37:17Z

And if somebody wants to look at PEFile, it is a simple field in the Module type. It is trivial to fetch it using regular windbg debugger commands.

We don't ship private symbols for desktop framework. This is something SOS/dac produces without having them.

I'm also in favor of removing clutter from commands, like displaying dwModuleID/dwModuleIndex only when non-zero. I just don't want to remove potentially useful bits for Desktop Framework as an accidental biproduct of changes here when avoidable.

jkotas · 2024-06-23T03:46:01Z

We don't ship private symbols for desktop framework. This is something SOS/dac produces without having them.

Can you do anything useful with PEAssembly pointer if you do not have private symbols? I think PEAssembly pointer is completely useless without private symbols.

leculver · 2024-06-23T15:49:15Z

I don't know all of the use cases of SOS. I'd simply prefer not to remove functionality if we don't have to.

You can still implement all of the .Net Core related functionality here without affecting Desktop Framework debugging.

leculver · 2024-06-23T16:59:34Z

Stepping back a second, the principle I'm trying to articulate is this:

When possible, please don't regress or take back Desktop Framework debugging features, as some of us still have to regularly debug that product. (Of course, I also mean when it wouldn't be too much work. I am not trying to hold anyone back.) As long as this version of SOS continues to support Desktop Framework, I think that's a reasonable position to take.

In this particular case, you are still able to make the changes you want to the output...it only means adding an if statement to maintain the status quo for Desktop. Feel free to change the text of "PEAssembly" to something more correct, always happy to have improvments. :)

I think that's a pretty reasonable (and not onerous) request, regardless of how useful the functionality is. Much appreciated!

jkotas · 2024-06-23T17:13:27Z

When possible, please don't regress or take back Desktop Framework debugging feature

I agree.

Feel free to change the text of "PEAssembly" to something more correct, always happy to have improvements. :)

Well, I am saying that the improvement would be to delete it unless somebody can explain why it is useful. This information was not in DumpModule output in the .NET Framework golden days and nobody missed it enough to add it to the output.

It was added to DumpModule output recently as part of 3000+ lines change that added support for ELF dumps: #124 . I am not sure why Mike added it. I do not see any paper trail that explains why it is useful to have this information in the DumpModule output.

mikem8361 · 2024-06-23T17:17:49Z

I don't remember why I added it of course. I'm completely ok with making these kind of SOS improvements.

mikem8361 · 2024-06-23T17:22:57Z

The SOS tests are failing because they expect a PEAssembly in the output. Line 91 in src\SOS\SOS.UnitTests\Scripts\OtherCommands.script needs to be removed.

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

elinor-fung · 2024-06-24T15:49:12Z

@leculver - completely agree with the higher level principle around Framework debugging experience and appreciate you calling it out here. For this particular case, per #4751 (comment) and #4751 (comment), this was a relatively recent addition that was never part of the original Framework experience, so I kept this PR as unconditionally removing the output.

Remove printing of DacpModuleData::PEAssembly from DumpModule command
6d69b01

elinor-fung requested review from jkotas and mikem8361 June 22, 2024 02:39

elinor-fung requested a review from a team as a code owner June 22, 2024 02:39

thaystg approved these changes Jun 22, 2024
View reviewed changes

jkotas approved these changes Jun 22, 2024
View reviewed changes

jkotas mentioned this pull request Jun 22, 2024
[cdac] start of RuntimeTypeSystem contract; implement GetMethodTableData SOS method dotnet/runtime#103444
Merged
2 tasks

leculver requested changes Jun 22, 2024
View reviewed changes

Comment threadsrc/SOS/Strike/strike.cpp

jkotas reviewed Jun 23, 2024
View reviewed changes

Comment threadsrc/SOS/Strike/strike.cpp Outdated

elinor-fungand others added 2 commits June 24, 2024 08:42

Update src/SOS/Strike/strike.cpp
fddb46e
Co-authored-by: Jan Kotas <jkotas@microsoft.com>

Update expected output in OtherCommands.script
a96f883

mikem8361 approved these changes Jun 24, 2024
View reviewed changes

leculver approved these changes Jun 24, 2024
View reviewed changes

This was referenced Jun 24, 2024
[test failure] SOS.WebApp3 fails on Alpine with VerifyOutput: no last command output or debugger exited unexpectedly: \s*Statistics:\s+ #4755
Closed
[Test] TestLogsUseAppFiltersAndFilterSpecs fails with Assert.Equal() Failure#4718
Closed

elinor-fung enabled auto-merge (squash) June 24, 2024 18:48

elinor-fung mentioned this pull request Jun 24, 2024
[test failure] TestCounterMonitorSystemRuntimeMetricsCSV intermittently fails: System.IO.EndOfStreamException : Attempted to read past the end of the stream. #4757
Open

elinor-fung merged commit 0ede436 into dotnet:mainJun 24, 2024

elinor-fung deleted the remove-peassembly branch June 24, 2024 18:57

elinor-fung mentioned this pull request Jun 28, 2024
[cdac] Implement !PrintExceptiondotnet/runtime#99302
Closed
25 tasks

github-actionsBot locked and limited conversation to collaborators Jul 26, 2024