Re: [Lldb-commits] [lldb] r303907 - Fix bug #28898

[Christos Zoulas via lldb-commits]

On May 31, 10:31am, lab...@google.com (Pavel Labath) wrote:
-- Subject: Re: [Lldb-commits] [lldb] r303907 - Fix bug #28898

| I hate to resurrect an old thread, but there has been a new spurt of
| this discussion about this patch here
| .
| 
| I think I have an idea on how to improve things slightly for us here,
| but as I know very little about this issue, I'd like someone to take a
| look at this first.
| 
| Christos, can you check whether my proposal makes sense to you? I am
| including it in this email for your convenience:
| 
| 
| ===
| I've re-read the last years discussions, and I think I have an idea on
| how to improve the situation here somewhat. The way I understand it,
| we have three scenarios to worry about:
| 
| 1. old-libedit --enable-widec
|   - el_gets -> narrow_read_function
|   - el_wgets -> wide_read_function
| 2. new-libedit --enable-widec
|   - el_gets -> wide_read_function
|   - el_wgets -> wide_read_function
| 3. libedit --disable-widec
|   - I don't actually know what happens here but my proposal does not
| change our behavior in this case.
| 
| As I understand it, the problem is that we are not able to distinguish
| between (1) and (2) and build time, so we have to just pick one and
| hope it works. This means we work correctly for (1), we fail for (2)
| and something unknown happens for (3).
| 
| However, we **are** able to distinguish between (1+2) and (3) at build
| time (just search for `el_wgets` symbol). And, if I understand it
| correctly, when we use el_wgets and friends we are always correct, no
| matter the libedit version. This would mean are always correct for
| both (1) **and** (2), while the situation remains unchanged for (3).
| This should be a strict improvement over status quo.

I think that you are right. Nevertheless what you propose it should
be an improvement over the status quo.

christos
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [lldb] r303907 - Fix bug #28898

[Christos Zoulas via lldb-commits]

On May 30,  6:02pm, ulrich.weig...@de.ibm.com ("Ulrich Weigand") wrote:
-- Subject: Re: [Lldb-commits] [lldb] r303907 - Fix bug #28898

| Hmm, from looking at the version of libedit I'm using it would
| appear that it should correctly handle either:
| - a narrow char read function installed via el=5Fset; or
| - a wide char read function installed via el=5Fwset
| which seems reasonable to me at first glance.

Yes, indeed.

| However, LLDB currently uses a wide char read function but installs
| it with el=5Fset, which is what breaks.  This would seem to be rather
| a LLDB problem, or am I missing something?

Newer version of libedit broke this compatibility:

el_gets -> narrow_read_function
el_wgets -> wide_read_function

and now they have:

el_{w,}gets -> wide_read_function

I think that the best way is to put it back (i.e. narrow libedit
functions require a narrow read function), and bump the version of libedit.

Of course that does not help people who have versions of libedit that
broke this compatibility (these define el_rfunc_t in histedit.h).

christos
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [lldb] r303907 - Fix bug #28898

[Christos Zoulas via lldb-commits]

On May 29,  8:11pm, ulrich.weig...@de.ibm.com ("Ulrich Weigand") wrote:
-- Subject: Re: [Lldb-commits] [lldb] r303907 - Fix bug #28898

| Sorry, this is on an internal IBM machine ... I don't have a publically
| accessible machine to reproduce this on at the moment.

I am sorry to say that this is my fault and this sucks. I accepted
some patches that broke binary compatibility in a way that it is not
user visible. Basically you seem to have version of libedit that is
between the narrow one that works, and the wide one that has the
binary compatibility of the wide code removed.

| I managed to debug a bit myself, and I think the problem is this:

Yes, this is correct. Basically your version of libedit handles
properly the narrow char read function and breaks if it is defined
as a wide char. if your histedit header does not have EL_ALIAS_TEXT
defined, you can check for that define too and not use the wide read
if that is not defined. Unfortunately there was no other change in
the version that would help differentiating the two libedit versions.

To fix this properly we would require an updated version of libedit.

christos
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D61191: Editline: Fix an msan error

[Christos Zoulas via lldb-commits]

On Apr 26,  3:11pm, revi...@reviews.llvm.org (Pavel Labath via Phabricator) 
wrote:
-- Subject: [PATCH] D61191: Editline: Fix an msan error

| 
| --b1_b299efcc557883c5ff30a5eebc16e12b
| Content-Type: text/plain; charset=us-ascii
| Content-Transfer-Encoding: quoted-printable
| 
| labath created this revision.
| labath added reviewers: christos, krytarowski, davide.
| 
| Despite the documentation for the el_get(EL_GETTC) function claiming the
| vararg part is (const char *name, void *value), in reality the function
| expects the vararg list to be terminated by a null pointer, which can be
| clearly seen by examining the source code. Although this is mostly
| bening because the extra values are not used it any way, it still lights
| up as an error when running the tests under msan.
| 
| Work around this quirk by adding an explicit nullptr to the end of the
| argument list.
| 
| 
| https://reviews.llvm.org/D61191

fixed, thanks!

christos
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D61191: Editline: Fix an msan error

[Christos Zoulas via lldb-commits]

Yes, you don't need to NULL terminate the argument list now.
Here's a link to the commit:
http://mail-index.netbsd.org/source-changes/2019/04/26/msg105454.html 


Best,

christos

> On Apr 29, 2019, at 7:39 AM, Pavel Labath  wrote:
> 
> On 26/04/2019 18:56, Christos Zoulas wrote:
>> On Apr 26,  3:11pm, revi...@reviews.llvm.org (Pavel Labath via Phabricator) 
>> wrote:
>> -- Subject: [PATCH] D61191: Editline: Fix an msan error
>> |
>> | --b1_b299efcc557883c5ff30a5eebc16e12b
>> | Content-Type: text/plain; charset=us-ascii
>> | Content-Transfer-Encoding: quoted-printable
>> |
>> | labath created this revision.
>> | labath added reviewers: christos, krytarowski, davide.
>> |
>> | Despite the documentation for the el_get(EL_GETTC) function claiming the
>> | vararg part is (const char *name, void *value), in reality the function
>> | expects the vararg list to be terminated by a null pointer, which can be
>> | clearly seen by examining the source code. Although this is mostly
>> | bening because the extra values are not used it any way, it still lights
>> | up as an error when running the tests under msan.
>> |
>> | Work around this quirk by adding an explicit nullptr to the end of the
>> | argument list.
>> |
>> |
>> | https://reviews.llvm.org/D61191
>> fixed, thanks!
>> christos
> 
> 
> Oooh, thanks for the super-fast fix.
> 
> Just to confirm, by "fixed" you mean that it should now not be needed to 
> terminate the vararg list with a null pointer?
> 
> Do you have anything (a revision, version number or something) that I can 
> leave as a trail to future maintainers to remove this workaround when the 
> fixed libedit becomes more widely available?
> 
> pl

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits