Page MenuHomePhabricator

hwasan: Initialize the pass only once.
ClosedPublic

Authored by pcc on Jul 16 2019, 5:37 PM.

Details

Summary

This will let us instrument globals during initialization. This required
making the new PM pass a module pass, which should still provide access to
analyses via the ModuleAnalysisManager.

Event Timeline

pcc created this revision.Jul 16 2019, 5:37 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 16 2019, 5:37 PM

I don't know what is the best practice here. Other sanitizers have both module and function passes, is there a reason to do things differently here?
@chandlerc

pcc added a comment.Jul 17 2019, 12:44 PM

IIUC the reason was to work around the lack of analysis availability from module passes, which is no longer a concern with the new PM. I would personally prefer to see the other sanitizers re-architected this way.

eugenis accepted this revision.Jul 17 2019, 1:32 PM

OK, sure.
As I understand functon passes also have better data locality by running a sequence of passes over a single function. Not sure how important that is.
LGTM

This revision is now accepted and ready to land.Jul 17 2019, 1:32 PM
This revision was automatically updated to reflect the committed changes.

cc: @fedor.sergeev @philip.pfaffe on this

I think one of the things we want in the new PM is the data locality that we see from iterating only specific IR units (functions in this case). This was brought up before when porting ASan (D54337). In this patch it also looks like the pass still goes over functions only. If there are other globals that this should pass over, could you add/update a test to show that?

pcc added a comment.Jul 17 2019, 3:34 PM

The instrumentation for globals is coming in a separate patch.

It seems more important to achieve implementation simplicity for the sanitizer passes rather than data locality because they make fundamental changes to the IR that touch the whole module (unlike other passes that are meant to preserve semantics). If the new PM is extended to allow whole-module changes within a function pass, I think the balance may change in favour of making this a function pass.