Skip to content

pkg/report: get only the thread local reports #6376

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

pkg/report: get only the thread local reports #6376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions pkg/report/linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,9 @@ func (ctx *linux) Parse(output []byte) *Report {
}
for questionable := false; ; questionable = true {
rep := &Report{
Output: output,
StartPos: startPos,
Output: output,
StartPos: startPos,
ContextID: context,
}
endPos, reportEnd, report, prefix := ctx.findReport(output, oops, startPos, context, questionable)
rep.EndPos = endPos
Expand Down
18 changes: 15 additions & 3 deletions pkg/report/report.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ type Report struct {
MachineInfo []byte
// If the crash happened in the context of the syz-executor process, Executor will hold more info.
Executor *ExecutorInfo
// On Linux systems ContextID may be the ThreadID(enabled by CONFIG_PRINTK_CALLER)
// or alternatively CpuID.
ContextID string

// reportPrefixLen is length of additional prefix lines that we added before actual crash report.
reportPrefixLen int
// symbolized is set if the report is symbolized. It prevents double symbolization.
Expand Down Expand Up @@ -277,9 +281,9 @@ func IsSuppressed(reporter *Reporter, output []byte) bool {
bytes.Contains(output, gceConsoleHangup)
}

// ParseAll returns all successive reports in output.
func ParseAll(reporter *Reporter, output []byte) (reports []*Report) {
skipPos := 0
// ParseAll returns all successive reports in output starting from startFrom.
func ParseAll(reporter *Reporter, output []byte, startFrom int) (reports []*Report) {
skipPos := startFrom
for {
rep := reporter.ParseFrom(output, skipPos)
if rep == nil {
Expand Down Expand Up @@ -958,3 +962,11 @@ func MergeReportBytes(reps []*Report) []byte {
func SplitReportBytes(data []byte) [][]byte {
return bytes.Split(data, []byte(reportSeparator))
}

func mergeReportContextIDs(reps []*Report) string {
var ids []string
for _, rep := range reps {
ids = append(ids, rep.ContextID)
}
return strings.Join(ids, ", ")
}
36 changes: 33 additions & 3 deletions pkg/report/report_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ type ParseTest struct {
HasReport bool
Report []byte
Executor string
ContextIDs string
// Only used in report parsing:
corruptedReason string
}
Expand Down Expand Up @@ -88,6 +89,9 @@ func (test *ParseTest) Headers(includeFrame bool) []byte {
if test.Executor != "" {
fmt.Fprintf(buf, "EXECUTOR: %s\n", test.Executor)
}
if test.ContextIDs != "" {
fmt.Fprintf(buf, "CONTEXTS: %s\n", test.ContextIDs)
}
return buf.Bytes()
}

Expand All @@ -96,8 +100,8 @@ func testParseFile(t *testing.T, reporter *Reporter, fn string) {
testParseImpl(t, reporter, test)
}

func parseReport(t *testing.T, reporter *Reporter, fn string) *ParseTest {
data, err := os.ReadFile(fn)
func parseReport(t *testing.T, reporter *Reporter, testFileName string) *ParseTest {
data, err := os.ReadFile(testFileName)
if err != nil {
t.Fatal(err)
}
Expand All @@ -110,7 +114,7 @@ func parseReport(t *testing.T, reporter *Reporter, fn string) *ParseTest {
)
phase := phaseHeaders
test := &ParseTest{
FileName: fn,
FileName: testFileName,
}
prevEmptyLine := false
s := bufio.NewScanner(bytes.NewReader(data))
Expand Down Expand Up @@ -158,6 +162,7 @@ func parseHeaderLine(t *testing.T, test *ParseTest, ln string) {
corruptedPrefix = "CORRUPTED: "
suppressedPrefix = "SUPPRESSED: "
executorPrefix = "EXECUTOR: "
contextidPrefix = "CONTEXTS: "
)
switch {
case strings.HasPrefix(ln, "#"):
Expand Down Expand Up @@ -193,6 +198,8 @@ func parseHeaderLine(t *testing.T, test *ParseTest, ln string) {
}
case strings.HasPrefix(ln, executorPrefix):
test.Executor = ln[len(executorPrefix):]
case strings.HasPrefix(ln, contextidPrefix):
test.ContextIDs = ln[len(contextidPrefix):]
default:
t.Fatalf("unknown header field %q", ln)
}
Expand Down Expand Up @@ -545,3 +552,26 @@ func TestSplitReportBytes(t *testing.T) {
})
}
}

// TestParseAll's focus points are the ability to:
// 1. parse multiple reports
// 2. extract correct ThreadID/CpuID.
func TestParseAll(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we parse all of our existing reports with this? How large will be the diff for existing reports? I think they generally did not include multiple reports, so perhaps not much will change. That may be better than creating 2 parallel test suites and 2 different tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This testing suite may benefit from the refactoring.
Do you feel concerned only about the parse_all or also about the guilty, guilty_raw, symbolize etc.?

I think all this transformation/extraction testing may be simplified a lot if we'll agree to have 2 files per test:
The code will be: Transform(input) -> serialize what we care about -> compare to the output file.
Current -update flag will work the same way allowing us to do the large scale tuning.

In this case we don't need any parsing logic. Only the function_under_test + logging the parts we care about.
WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you feel concerned only about the parse_all or also about the guilty, guilty_raw, symbolize etc.?

guilty accepts different inputs (symbolized and cleaned output, basically reports).
I dunno what is symbolize, and why it's separated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One file per test was intentional, it makes diff reviews possible.
For example, if we split them into 2 files, if somebody changes something that changes output, PR will contain only the output files, but it will be hard to understand if these are legit changes or not, b/c you won't see the input files in the PR.

forEachFile(t, "parse_all", testParseAll)
}

func testParseAll(t *testing.T, reporter *Reporter, testFileName string) {
test := parseReport(t, reporter, testFileName)
gotReports := ParseAll(reporter, test.Log, 0)
gotIDs := mergeReportContextIDs(gotReports)
mergedReport := MergeReportBytes(gotReports)
if !bytes.Equal(mergedReport, test.Report) || gotIDs != test.ContextIDs {
if *flagUpdate {
updateReportTest(t, test, &ParseTest{
ContextIDs: gotIDs,
Report: mergedReport})
}
assert.Equal(t, test.ContextIDs, gotIDs, "extracted wrong Thread or CPU ids")
assert.Equal(t, string(test.Report), string(mergedReport), "extracted wrong reports")
}
}
836 changes: 836 additions & 0 deletions pkg/report/testdata/linux/parse_all/1
View file
Open in desktop

Large diffs are not rendered by default.

Loading