[lld] findAbsoluteAtom asserts the atom is really existing

[davide]

Summary

While working on the crash triggered by the testcase reported by Rafael in "[lld] r227711 - [test] Add test for section groups and deadstrip", I stumbled upon this.
It looks like findAbsoluteAtom() does not check if the atom is actually found, neither do some of its consumers, and this might cause a NULL pointer derefence. I think it's legit to ask to the consumers to be sure about what they're looking for, so I propose to add an assert to absoluteAtom(). An alternative is that of pushing the check in the consumers of findAbsoluteAtom() but I'm more in favour of this centralized approach, unless there are some cases in which is actually possible for some consumer to specify an element that might not belong to the underlying vector of Atoms.

[atanasyan]

I think it is rather unusual semantics for a find function to check its result inside itself. It is necessary to have a function which really find an atom by its name and allows caller to check the atom existence. For example MIPS object files usually define _gp symbol but this symbol is not mandatory. If we need a function returns an absolute atom by its name when we sure that the atom exists we can introduce a new function getAbsoluteAtom, put the assert call into it and replace findAbsoluteAtom by this new function where it is applicable.

[davide]

Your use case sounds reasonable and, maybe, we should move the check in the caller. I would like to hear what other people think, in particular Shankar who originally introduced this code. Another possibility to avoid introducing getAbsoluteAtom() might be that of passing a boolean flag to the existing function to change the behaviour (assert/not assert). I'm not sure I like this a lot, though.

[• shankar.easwaran]

Find should not check the result in the function. I like the approach that Simon suggested.

[ruiu]

I agree with Simon. Adding a boolean flag to the existing function does not sounds like a good idea, just because findAbsoluteAtom(s, true /* dieNotFound */) doesn't look better than getAbsoluteAtom(s).

[davide]

Abandon this, r230614 fixed the problem in a different way.

DefaultLayout.h

 
   /// \brief find a absolute atom given a name
   AbsoluteAtomIterT findAbsoluteAtom(StringRef name) {
-    return std::find_if(_absoluteAtoms.begin(), _absoluteAtoms.end(),
+    auto ret = std::find_if(_absoluteAtoms.begin(), _absoluteAtoms.end(),
                         FindByName(name));
+    assert (ret != _absoluteAtoms.end());
+    return ret;
   }
 
   // Output sections with the same name into a OutputSection