Page MenuHomePhabricator

[DOC] Update developer notes
ClosedPublic

Authored by Fabien on Mar 2 2020, 15:20.

Details

Summary

This makes the instructions use cmake/ninja and adds some details when
necessary, especially for running the sanitizers as requested in D5391.

Depends on D5321.

Test Plan

Read the doc and try the instructions.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
doc_developer_notes
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9750
Build 17380: Default Diff Build & Tests
Build 17379: arc lint + arc unit

Event Timeline

deadalnix added inline comments.
doc/developer-notes.md
392 ↗(On Diff #16686)

I literally have zero failure from ubsan. Is that necessary?

doc/developer-notes.md
392 ↗(On Diff #16686)

The suppression list probably deserves an update, since it has been extracted from core some time ago.

But there are cases that will only trigger with some compilers and compiler versions (e.g. there is a vptr reported in avalanche_tests with clang-7), so I'd better leave it to prevent people from facing issues.

doc/developer-notes.md
364 ↗(On Diff #16686)

Why is debug necessary here?

doc/developer-notes.md
364 ↗(On Diff #16686)

It is not necessary, but can help in having a better trace if an failure is triggered.

doc/developer-notes.md
361 ↗(On Diff #16686)

IMO these could just be added by the sanitizer module to some place and the test module can query these. There is no reason for people to have to deal with this.

392 ↗(On Diff #16686)

We already require clang format 8, and this is packaged in most distros. We don't support 7 and less. Backing old stuff in there is just asking for regressions.

doc/developer-notes.md
244 ↗(On Diff #16686)

There is one target per test now and it can be used instead of this.

Use the dedicated target for building and running script_tests.

jasonbcox added inline comments.
doc/developer-notes.md
311 ↗(On Diff #16721)

Doesn't clang need to be used as well? My understanding is sanitizer support exists for gcc but we do not support it.
Same comment/question for the examples below.

Fabien planned changes to this revision.Mar 3 2020, 20:32
Fabien added inline comments.
doc/developer-notes.md
311 ↗(On Diff #16721)

AFAIK both can work, it's more a matter of version.
But since we are using clang, I can add a note to state that it should preferably run with clang >8, so it will address a previous comment from @deadalnix at the same time.

Add a recommendation for using clang over gcc with sanitizers.
No clang version but "recent" in order to reduce maintenance of the doc.

doc/developer-notes.md
332 ↗(On Diff #16747)

Why would we limit the sanitizer uses to clang?

342 ↗(On Diff #16747)

Write paragraph as paragraphs.

381 ↗(On Diff #16747)

I will reiterate that it would be very beneficial that this is done automatically when the sanitizer is enabled.

Fix the suppression files path, actually make the paragraph look like a paragraph.

doc/developer-notes.md
332 ↗(On Diff #16747)

That's just what we are using on CI, as we had various issues with GCC in the past (I don't remember the details, I think there was some difference in the suppression file with some sanitizers, and some false positive on system libraries).
I can certainly give it another try, the suppression list deserves an update anyway.

381 ↗(On Diff #16747)

I agree it would be a good improvement, but it's non trivial and out of scope for this diff.

deadalnix requested changes to this revision.Mar 6 2020, 16:18
deadalnix added inline comments.
doc/developer-notes.md
334

I think this still need to be changed. This convey the idea that our goal is to be green in clang, whereas it is to be clean with gcc too.

This revision now requires changes to proceed.Mar 6 2020, 16:18

Remove the advice for using Clang.

This revision is now accepted and ready to land.Mar 6 2020, 22:02
This revision was automatically updated to reflect the committed changes.