Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More comprehensive and deterministic reporting #78

Merged
merged 13 commits into from
May 14, 2024

Conversation

fosterbrereton
Copy link
Contributor

@fosterbrereton fosterbrereton commented May 8, 2024

This PR includes a wide number of changes to improve the determinism and breadth of ODRV reporting. The reports are becoming quite dense, but that is purposeful to maximize a report's signal to noise ratio. To make it easier to explain what's all in here, let's break down a sample report:

error: ODRV (member:data_member_location, member:type); 4 conflicts with `example::symbol`
    name: symbol
    type: `unsigned short***`
    data_member_location: 56 (0x38)
    symbol defintion location(s):
        /path/to/lib16/lib16.h:818 (used by `somelib.a -> a16.o` and 4 others)

    name: symbol
    type: `unsigned short***`
    data_member_location: 64 (0x40)
    symbol defintion location(s):
        /path/to/lib16/lib16.h:818 (used by `otherlib16.a -> b.o` and 55 others)

    name: symbol
    type: `short***`
    data_member_location: 56 (0x38)
    symbol defintion location(s):
        /path/to/lib12/lib12.h:813 (used by `somelib.a -> a12.o` and 1 others)

    name: symbol
    type: `unsigned char***`
    data_member_location: 56 (0x38)
    symbol defintion location(s):
        /path/to/lib8/lib8.h:813 (used by `somelib.a -> c.o` and 2 others)


error: ODRV (member:data_member_location); 2 conflicts with `example::other_symbol`
    name: other_symbol
    type: `short***`
    data_member_location: 72 (0x48)
    symbol defintion location(s):
        /path/to/lib16/lib16.h:823 (used by `otherlib16.a -> a.o` and 55 others)

    name: other_symbol
    type: `short***`
    data_member_location: 64 (0x40)
    symbol defintion location(s):
        /path/to/lib16/lib16.h:823 (used by `somelib.a -> a16.o` and 4 others)
        /path/to/lib12/lib12.h:818 (used by `somelib.a -> a12.o` and 1 others)
        /path/to/lib8/lib8.h:818 (used by `somelib.a -> c.o` and 2 others)

Filtered out 52 ODRVs in the following categories:
    structure:calling_convention
    subprogram:accessibility
    subprogram:artificial
    subprogram:explicit_
    typedef:type

Here are the most significant changes:

  • We now report on multiple categories, not just one. For a given symbol, it is possible to have definitions that conflict in multiple categories. Previously we'd pick only one category and decide if the ODRV should be filtered out or reported on, even if other categories should be reported on. The new reports now list all the categories where there are any conflicts. (In the case above, member:data_member_location, member:type).
  • We enumerate the symbol definition location(s). ODR does allow for symbols to be defined in different places with some strict requirements. So it is not an ODRV necessarily when definitions are sourced from different places. However, when we do find an ODRV, we can separate those definitions based on how they differ. We also include an example object file that uses an instance of this symbol definition, along with a count of other instances that use it, too.
  • We enumerate the ODRVs that were filtered out. This gives report readers confidence that there are no false negatives slipping by & allowing them to fine-tune ORC's reporting settings.

@fosterbrereton fosterbrereton marked this pull request as ready for review May 9, 2024 23:11
@@ -22,11 +22,20 @@
struct odrv_report {
odrv_report(std::string_view symbol, const die* list_head);

std::string category() const;
std::size_t category_count() const { return _conflicting_attributes.size(); }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes reflect the increased complexity around ODRV category management.

@@ -67,41 +67,59 @@ std::string_view path_to_symbol(std::string_view path) {
}

/**************************************************************************************************/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes refactor find_attribute_conflict to make it easier to understand.

const auto conflict_first = _conflict_map.begin();
const auto conflict_last = _conflict_map.end();

for (auto x = conflict_first; x != conflict_last; ++x) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of comparing just two entries in the conflict map, now we compare all of them to each other, collecting all the categories where they conflict along the way.

include/orc/orc.hpp Show resolved Hide resolved
src/dwarf.cpp Show resolved Hide resolved
src/orc.cpp Outdated Show resolved Hide resolved
src/orc.cpp Outdated Show resolved Hide resolved
@leethomason
Copy link
Contributor

@fosterbrereton I checked the code with "deterministic output" in mind and didn't see any problems. But wanted to confirm that you've run this against our massive codebase and ORC still produces the same text with every run?

@fosterbrereton
Copy link
Contributor Author

fosterbrereton commented May 11, 2024

@fosterbrereton I checked the code with "deterministic output" in mind and didn't see any problems. But wanted to confirm that you've run this against our massive codebase and ORC still produces the same text with every run?

I have not run this over our code base yet. I'll find some time to do that this upcoming week.

@fosterbrereton
Copy link
Contributor Author

@leethomason I have run this PR over our sources and am seeing the same output being produced from the same input over multiple runs of the tool.

Copy link
Contributor

@leethomason leethomason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - nice set of changes.

@fosterbrereton fosterbrereton merged commit 0c5c524 into main May 14, 2024
3 checks passed
@fosterbrereton fosterbrereton deleted the fbrereto/improved-reporting branch May 14, 2024 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants