Page MenuHomePhabricator

[ELF] Initialize 2 fields of Symbol in SymbolTable::insert
ClosedPublic

Authored by MaskRay on Tue, Aug 13, 2:32 AM.

Details

Summary

A new symbol is added to elf::symtab in 3 steps:

  1. SymbolTable::insert creates a placeholder.
  2. Symbol::mergeProperties
  3. Symbol::replace

Fields referenced by steps 2) and 3) should be initialized in
SymbolTable::insert. traced and referenced were missed previously.
This did not cause problems because compilers generated code that
initialized them (bit fields) to 0.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Tue, Aug 13, 2:32 AM
grimar accepted this revision.Tue, Aug 13, 2:38 AM

LGTM. Please wait for other reviewer(s) opinion about comments (I'd remove them).

ELF/SymbolTable.cpp
75 ↗(On Diff #214787)

I am not sure that comments of this patch are valuable. You just initializing fields
because they can be used anywhere in LLD. Not sure it is worth to mention where
they are used/copied.

This revision is now accepted and ready to land.Tue, Aug 13, 2:38 AM
MaskRay updated this revision to Diff 214789.Tue, Aug 13, 2:51 AM

Improve comments

LGTM too, I agree with George that making the comment specific isn't helpful but a general one isn't doing any harm.

An alternative is to make an initialization function in Symbol and have the default constructor call that for the bitfields. This could be called from SymbolTable::insert. This would have the benefit that we'd only have to update one place for a new field. I notice that canInline is given a different default value from the default constructor, but if this is important it could be a parameter to the initialization function.

A further alternative is to wrap the bitfields in a struct that can be initialized and copied as a whole, but would require every use of one of the fields to be updated.

LGTM too, I agree with George that making the comment specific isn't helpful but a general one isn't doing any harm.

An alternative is to make an initialization function in Symbol and have the default constructor call that for the bitfields. This could be called from SymbolTable::insert. This would have the benefit that we'd only have to update one place for a new field. I notice that canInline is given a different default value from the default constructor, but if this is important it could be a parameter to the initialization function.

Yes, canInline is weird. The member initializer arbitrarily sets it to false, then SymbolTable::insert changes it to true, in --wrap handling it gets assigned to false (true->false semilattice).

A further alternative is to wrap the bitfields in a struct that can be initialized and copied as a whole, but would require every use of one of the fields to be updated.

The problem is that I can't give the member function a reasonable name :( Conceptually any field referenced when the symbol is still a placeholder should be initialized (these fields are all bit fields currently). Note, there are many other fields that are not referenced - they are not initialized in SymbolTable::insert. Some bit fields are initialized differently in the member initializer list and SymbolTable::insert

  • visibility
  • isUsedInRegularObj
  • canInline
  • exportDynamic

So a member function may not save us a lot of trouble.

MaskRay updated this revision to Diff 214794.Tue, Aug 13, 3:16 AM

Delete a sentence from a comment

This revision was automatically updated to reflect the committed changes.